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

[Momchil Velikov via Phabricator via cfe-commits]

chill marked 4 inline comments as done.
chill added inline comments.



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

snidertm wrote:
> 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
CallInst *CI = ... ;
   ... = CI->getAttributes().hasFnAttribute("cmse_nonsecure_call"))


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

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

In D71129#1956137 , @snidertm wrote:

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


The last two CMSE patches are under review: https://reviews.llvm.org/D76518


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

[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

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

[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

[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

[Momchil Velikov via Phabricator via cfe-commits]

chill marked 2 inline comments as done.
chill 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; }
 

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.


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

[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

[Momchil Velikov via Phabricator via cfe-commits]

chill marked 3 inline comments as done.
chill added inline comments.



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

snidertm wrote:
> 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
Hmm, I think 8 is OK, but we should mask it ...



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

... here.

   bool getHasRegParm() const { return ((Bits & RegParmMask) >> RegParmOffset) 
!= 0;


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

[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

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

[Momchil Velikov via cfe-commits]

From: Russell Gallop 
> The new case that you've added to save-temps.c doesn't work if you only build 
> with
> DLLVM_TARGETS_TO_BUILD=X86 (i.e. not building ARM). Please could you take a 
> look?

Yes, I've been looking into it. I may revert that test alone, until I figure 
out how to fix it. https://reviews.llvm.org/D76695
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Russell Gallop via cfe-commits]

Hi Momchil,

Can I just check that you've seen this bot failure here since this patch:
http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/64604/steps/test-check-all/logs/FAIL%3A%20Clang%3A%3Asave-temps.c


The new case that you've added to save-temps.c doesn't work if you only
build with -DLLVM_TARGETS_TO_BUILD=X86 (i.e. not building ARM).
Please could you take a look?

Thanks
Russ

On Tue, 24 Mar 2020 at 10:45, Momchil Velikov via Phabricator via
cfe-commits  wrote:

> This revision was automatically updated to reflect the committed changes.
> Closed by commit rG080d046c91d2: [ARM][CMSE] Implement CMSE attributes
> (authored by chill).
> Herald added a project: clang.
> Herald added a subscriber: cfe-commits.
>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D71129/new/
>
> https://reviews.llvm.org/D71129
>
> Files:
>   clang/include/clang/AST/Type.h
>   clang/include/clang/AST/TypeProperties.td
>   clang/include/clang/Basic/Attr.td
>   clang/include/clang/Basic/AttrDocs.td
>   clang/include/clang/Basic/DiagnosticDriverKinds.td
>   clang/include/clang/Basic/DiagnosticSemaKinds.td
>   clang/include/clang/CodeGen/CGFunctionInfo.h
>   clang/lib/AST/TypePrinter.cpp
>   clang/lib/CodeGen/CGCall.cpp
>   clang/lib/Driver/ToolChains/Clang.cpp
>   clang/lib/Sema/SemaDecl.cpp
>   clang/lib/Sema/SemaDeclAttr.cpp
>   clang/lib/Sema/SemaExpr.cpp
>   clang/lib/Sema/SemaType.cpp
>   clang/lib/Serialization/ASTWriter.cpp
>   clang/test/AST/ast-dump-arm-attr.c
>   clang/test/CodeGen/arm-cmse-attr.c
>   clang/test/CodeGen/arm-cmse-call.c
>   clang/test/Driver/ropi-rwpi.c
>   clang/test/Driver/save-temps.c
>   clang/test/Misc/pragma-attribute-supported-attributes-list.test
>   clang/test/Sema/arm-cmse.c
>   clang/test/Sema/arm-no-cmse.c
>   clang/test/SemaCXX/arm-cmse.cpp
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
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

[Momchil Velikov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG080d046c91d2: [ARM][CMSE] Implement CMSE attributes 
(authored by chill).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71129

Files:
  clang/include/clang/AST/Type.h
  clang/include/clang/AST/TypeProperties.td
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/CodeGen/CGFunctionInfo.h
  clang/lib/AST/TypePrinter.cpp
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaType.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/AST/ast-dump-arm-attr.c
  clang/test/CodeGen/arm-cmse-attr.c
  clang/test/CodeGen/arm-cmse-call.c
  clang/test/Driver/ropi-rwpi.c
  clang/test/Driver/save-temps.c
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/arm-cmse.c
  clang/test/Sema/arm-no-cmse.c
  clang/test/SemaCXX/arm-cmse.cpp

Index: clang/test/SemaCXX/arm-cmse.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/arm-cmse.cpp
@@ -0,0 +1,5 @@
+// RUN: %clang_cc1 -triple thumbv8m.base-none-eabi -mcmse -verify %s
+
+extern "C" void foo() __attribute__((cmse_nonsecure_entry)) {}
+
+void bar() __attribute__((cmse_nonsecure_entry)) {} // expected-error{{function type with 'cmse_nonsecure_entry' attribute must have C linkage}}
Index: clang/test/Sema/arm-no-cmse.c
===
--- /dev/null
+++ clang/test/Sema/arm-no-cmse.c
@@ -0,0 +1,7 @@
+// RUN: %clang_cc1 -triple thumbv8m.base-none-eabi -verify %s
+
+typedef void (*callback_ns_1t)()
+  __attribute__((cmse_nonsecure_call)); // expected-warning{{'cmse_nonsecure_call' attribute ignored}}
+
+void f()
+  __attribute__((cmse_nonsecure_entry)) {} // expected-warning{{'cmse_nonsecure_entry' attribute ignored}}
Index: clang/test/Sema/arm-cmse.c
===
--- /dev/null
+++ clang/test/Sema/arm-cmse.c
@@ -0,0 +1,30 @@
+// RUN: %clang_cc1 -triple thumbv8m.base-none-eabi -mcmse -verify %s
+
+typedef void (*callback_ns_1t)() __attribute__((cmse_nonsecure_call));
+typedef void (*callback_1t)();
+typedef void (*callback_ns_2t)() __attribute__((cmse_nonsecure_call));
+typedef void (*callback_2t)();
+
+void foo(callback_ns_1t nsfptr, // expected-error{{functions may not be declared with 'cmse_nonsecure_call' attribute}}
+ callback_1t fptr) __attribute__((cmse_nonsecure_call))
+{
+  callback_1t fp1 = nsfptr; // expected-warning{{incompatible function pointer types initializing 'callback_1t'}}
+  callback_ns_1t fp2 = fptr; // expected-warning{{incompatible function pointer types initializing 'callback_ns_1t'}}
+  callback_2t fp3 = fptr;
+  callback_ns_2t fp4 = nsfptr;
+}
+
+static void bar() __attribute__((cmse_nonsecure_entry)) // expected-warning{{'cmse_nonsecure_entry' cannot be applied to functions with internal linkage}}
+{
+}
+
+typedef void nonsecure_fn_t(int) __attribute__((cmse_nonsecure_call));
+extern nonsecure_fn_t baz; // expected-error{{functions may not be declared with 'cmse_nonsecure_call' attribute}}
+
+int v0 __attribute__((cmse_nonsecure_call)); // expected-warning {{'cmse_nonsecure_call' only applies to function types; type here is 'int'}}
+int v1 __attribute__((cmse_nonsecure_entry)); // expected-warning {{'cmse_nonsecure_entry' attribute only applies to functions}}
+
+void fn0() __attribute__((cmse_nonsecure_entry));
+void fn1() __attribute__((cmse_nonsecure_entry(1)));  // expected-error {{'cmse_nonsecure_entry' attribute takes no arguments}}
+
+typedef void (*fn2_t)() __attribute__((cmse_nonsecure_call("abc"))); // expected-error {{'cmse_nonsecure_call' attribute takes no argument}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -39,6 +39,7 @@
 // CHECK-NEXT: Callback (SubjectMatchRule_function)
 // CHECK-NEXT: Capability (SubjectMatchRule_record, SubjectMatchRule_type_alias)
 // CHECK-NEXT: CarriesDependency (SubjectMatchRule_variable_is_parameter, SubjectMatchRule_objc_method, SubjectMatchRule_function)
+// CHECK-NEXT: CmseNSEntry (SubjectMatchRule_function)
 // CHECK-NEXT: Cold (SubjectMatchRule_function)
 // CHECK-NEXT: Common (SubjectMatchRule_variable)
 // CHECK-NEXT: ConstInit (SubjectMatchRule_variable_is_global)
Index: clang/test/Driver/save-temps.c