[PATCH] D59879: [ARM][CMSE] Add commandline option and feature macro

2019-03-28 Thread Todd Snider via Phabricator via cfe-commits
snidertm added inline comments.



Comment at: lib/Basic/Targets/ARM.cpp:438
+} else if (Feature == "+8msecext") {
+  if ((CPUProfile != "M" && CPUProfile != "B") || ArchVersion != 8) {
+Diags.Report(diag::err_target_unsupported_mcmse) << CPU;

How does CPUProfile get a value of "B"? I thought any Cortex-M processor would 
set CPUProfile to "M". Is CMSE available on a processor besides Cortex-m33?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59879/new/

https://reviews.llvm.org/D59879



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D59879: [ARM][CMSE] Add commandline option and feature macro

2019-03-29 Thread Todd Snider via Phabricator via cfe-commits
snidertm added inline comments.



Comment at: lib/Basic/Targets/ARM.cpp:438
+} else if (Feature == "+8msecext") {
+  if ((CPUProfile != "M" && CPUProfile != "B") || ArchVersion != 8) {
+Diags.Report(diag::err_target_unsupported_mcmse) << CPU;

dmgreen wrote:
> snidertm wrote:
> > How does CPUProfile get a value of "B"? I thought any Cortex-M processor 
> > would set CPUProfile to "M". Is CMSE available on a processor besides 
> > Cortex-m33?
> "B" was going to be a very old name for v8m-baseline, looks like this one was 
> never cleaned up when that was changed. You can drop the != "B" check.
Do all v8 profile == 'M' processor variants support security extensions?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59879/new/

https://reviews.llvm.org/D59879



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D71129: [ARM][CMSE] Implement CMSE attributes

2020-04-01 Thread Todd Snider via Phabricator via cfe-commits
snidertm added inline comments.



Comment at: clang/include/clang/AST/Type.h:3588
+  NoCallerSavedRegsMask | NoCfCheckMask | CmseNSCallMask),
   RegParmOffset = 8
 }; // Assumed to be the last field

Shouldn't RegParmOffset be updated to 9, I believe it is used to shift the 
regParm value so that it encoded in the bits above CmseNSCallMask


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71129/new/

https://reviews.llvm.org/D71129



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D71129: [ARM][CMSE] Implement CMSE attributes

2020-04-01 Thread Todd Snider via Phabricator via cfe-commits
snidertm added inline comments.



Comment at: clang/include/clang/AST/Type.h:3604
+ (noCallerSavedRegs ? NoCallerSavedRegsMask : 0) |
+ (hasRegParm ? ((regParm + 1) << RegParmOffset) : 0) |
+ (NoCfCheck ? NoCfCheckMask : 0) |

Wouldn't this overwrite the CmseNSCallMask bit if hasRegParm is true and 
((regParm + 1) & 1) is non-zero?



Comment at: clang/include/clang/AST/Type.h:3622
 bool getNoCfCheck() const { return Bits & NoCfCheckMask; }
 bool getHasRegParm() const { return (Bits >> RegParmOffset) != 0; }
 

chill wrote:
> ... here.
> 
>bool getHasRegParm() const { return ((Bits & RegParmMask) >> 
> RegParmOffset) != 0;
I don't see how this helps. If RegParmOffset is 8 and the CmseNSCall bit is set 
in Bits, then your proposed getHasRegParm() will return true. Given the above 
assignment to Bits, we don't know if the CmseNSCall bit was set by cmseNSCall 
or by regParm.

MIght I be missing something?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71129/new/

https://reviews.llvm.org/D71129



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D71129: [ARM][CMSE] Implement CMSE attributes

2020-04-01 Thread Todd Snider via Phabricator via cfe-commits
snidertm added inline comments.



Comment at: clang/include/clang/AST/Type.h:3622
 bool getNoCfCheck() const { return Bits & NoCfCheckMask; }
 bool getHasRegParm() const { return (Bits >> RegParmOffset) != 0; }
 

chill wrote:
> snidertm wrote:
> > chill wrote:
> > > ... here.
> > > 
> > >bool getHasRegParm() const { return ((Bits & RegParmMask) >> 
> > > RegParmOffset) != 0;
> > I don't see how this helps. If RegParmOffset is 8 and the CmseNSCall bit is 
> > set in Bits, then your proposed getHasRegParm() will return true. Given the 
> > above assignment to Bits, we don't know if the CmseNSCall bit was set by 
> > cmseNSCall or by regParm.
> > 
> > MIght I be missing something?
> No, it will not return true, because the mask will clear all bits, except 
> bits [8-10,13-31].
> Bits [13-31] are unused/zero, and in the patch I'm preparing, the RegParmMask 
> will be simply 0x700,
> so they will be cleared anyway.
> CmseNSCall is bit 12, so it will be cleared. Also RegParm + 1 is at most 7, 
> so, it cannot overflow into
> NoCfCheck of CmseNSCall.
Got it, ok.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71129/new/

https://reviews.llvm.org/D71129



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D71129: [ARM][CMSE] Implement CMSE attributes

2020-04-01 Thread Todd Snider via Phabricator via cfe-commits
snidertm added a comment.

Have you already committed support for CMSE attributes to the LLVM backend? Or 
is that on the way?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71129/new/

https://reviews.llvm.org/D71129



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D71129: [ARM][CMSE] Implement CMSE attributes

2020-04-01 Thread Todd Snider via Phabricator via cfe-commits
snidertm added inline comments.



Comment at: clang/lib/CodeGen/CGCall.cpp:1882
+  if (FI.isCmseNSCall())
+FuncAttrs.addAttribute("cmse_nonsecure_call");
+

Just curious … Does the LLVM backend have a way to extract a StringRef 
attribute from a CallInst? I know that you can do that with hasFnAttribute(), 
but I don't see anything for CallInst objects


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71129/new/

https://reviews.llvm.org/D71129



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits