[PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

2020-09-16 Thread Daniel Kiss via Phabricator via cfe-commits
danielkiss updated this revision to Diff 292250.
danielkiss added a comment.

Rebased top of D85649 .


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

https://reviews.llvm.org/D80791

Files:
  llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-0.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-1.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-2.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-3.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-4.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-5.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-6.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-7.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-8.ll

Index: llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-8.ll
===
--- llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-8.ll
+++ /dev/null
@@ -1,21 +0,0 @@
-; RUN: llc -mtriple=aarch64-linux %s   -o - | \
-; RUN:   FileCheck %s --check-prefix=ASM
-; RUN: llc -mtriple=aarch64-linux %s -filetype=obj -o - | \
-; RUN:   llvm-readelf --notes - | FileCheck %s --check-prefix=OBJ
-
-define dso_local i32 @f() #0 {
-entry:
-  %r = tail call i32 @g()
-  ret i32 %r
-}
-
-declare dso_local i32 @g()
-
-attributes #0 = { "branch-target-enforcement"="true" }
-
-; Declarations don't prevent setting BTI
-; ASM:	.word	3221225472
-; ASM-NEXT:	.word	4
-; ASM-NEXT:	.word	1
-
-; OBJ: Properties: aarch64 feature: BTI
Index: llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-7.ll
===
--- llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-7.ll
+++ /dev/null
@@ -1,23 +0,0 @@
-; RUN: llc -mtriple=aarch64-linux %s   -o - 2>&1 | \
-; RUN:   FileCheck %s --check-prefix=ASM
-; RUN: llc -mtriple=aarch64-linux %s -filetype=obj -o -  |  \
-; RUN:   llvm-readelf -S - | FileCheck %s --check-prefix=OBJ
-
-define dso_local i32 @f() #0 {
-entry:
-  ret i32 0
-}
-
-define dso_local i32 @g() #1 {
-entry:
-  ret i32 0
-}
-
-attributes #0 = { "sign-return-address"="non-leaf" }
-
-attributes #1 = { "branch-target-enforcement"="true" }
-
-; No common attribute, no note section
-; ASM: warning: not setting BTI in feature flags
-; ASM-NOT: .note.gnu.property
-; OBJ-NOT: .note.gnu.property
Index: llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-6.ll
===
--- llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-6.ll
+++ /dev/null
@@ -1,22 +0,0 @@
-; RUN: llc -mtriple=aarch64-linux %s   -o - | \
-; RUN:   FileCheck %s --check-prefix=ASM
-; RUN: llc -mtriple=aarch64-linux %s -filetype=obj -o - |  \
-; RUN:   llvm-readelf -S - | FileCheck %s --check-prefix=OBJ
-
-define dso_local i32 @f() #0 {
-entry:
-  ret i32 0
-}
-
-define dso_local i32 @g() #1 {
-entry:
-  ret i32 0
-}
-
-attributes #0 = { "sign-return-address"="non-leaf" }
-
-attributes #1 = { "sign-return-address"="none" }
-
-; No common attribute, no note section
-; ASM-NOT: .note.gnu.property
-; OBJ-NOT: .note.gnu.property
Index: llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-5.ll
===
--- llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-5.ll
+++ /dev/null
@@ -1,26 +0,0 @@
-; RUN: llc -mtriple=aarch64-linux %s   -o - 2>&1 | \
-; RUN:   FileCheck %s --check-prefix=ASM
-; RUN: llc -mtriple=aarch64-linux %s -filetype=obj -o -  |  \
-; RUN:   llvm-readelf --notes - | FileCheck %s --check-prefix=OBJ
-
-define dso_local i32 @f() #0 {
-entry:
-  ret i32 0
-}
-
-define dso_local i32 @g() #1 {
-entry:
-  ret i32 0
-}
-
-attributes #0 = { "branch-target-enforcement"="true" "sign-return-address"="non-leaf" }
-
-attributes #1 = { "sign-return-address"="all" }
-
-; Only the common atttribute (PAC)
-; ASM: warning: not setting BTI in feature flags
-; ASM:	.word	3221225472
-; ASM-NEXT:	.word	4
-; ASM-NEXT:	.word	2
-
-; OBJ: Properties: aarch64 feature: PAC
Index: llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-4.ll
===
--- llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-4.ll
+++ llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-4.ll
@@ -1,7 +1,5 @@
 ; RUN: llc -mtriple=aarch64-linux %s   -o - | \
 ; RUN:   FileCheck %s --check-prefix=ASM
-; RUN: llc -mtriple=aarch64-linux %s -filetype=obj -o - |  \
-; RUN:   llvm-readelf --notes - | FileCheck %s --check-prefix=OBJ
 
 define dso_local i32 @f() #0 {
 entry:
@@ -17,9 +15,5 @@
 
 attributes #1 = { "branch-target-enforcement"="true" }
 
-; Only the common atttribute (BTI)
-; ASM:	.word	3221225472
-; ASM-NEXT:	.word	4
-; ASM-NEXT:	.word	1
-
-; OBJ: Properties: aarch64 feature: BTI
+; Note is not emited if module has no properties
+; ASM-NOT: 

[PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

2020-09-02 Thread Momchil Velikov via Phabricator via cfe-commits
chill added a comment.

LGTM, as soon as D85649  is accepted (so they 
stay in sync).


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

https://reviews.llvm.org/D80791

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


[PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

2020-08-29 Thread Daniel Kiss via Phabricator via cfe-commits
danielkiss updated this revision to Diff 288775.
danielkiss edited the summary of this revision.
danielkiss added a comment.

Sync with D85649 .


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

https://reviews.llvm.org/D80791

Files:
  clang/lib/CodeGen/CodeGenModule.cpp
  llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-0.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-1.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-2.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-3.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-4.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-5.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-6.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-7.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-8.ll

Index: llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-8.ll
===
--- llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-8.ll
+++ /dev/null
@@ -1,21 +0,0 @@
-; RUN: llc -mtriple=aarch64-linux %s   -o - | \
-; RUN:   FileCheck %s --check-prefix=ASM
-; RUN: llc -mtriple=aarch64-linux %s -filetype=obj -o - | \
-; RUN:   llvm-readelf --notes - | FileCheck %s --check-prefix=OBJ
-
-define dso_local i32 @f() #0 {
-entry:
-  %r = tail call i32 @g()
-  ret i32 %r
-}
-
-declare dso_local i32 @g()
-
-attributes #0 = { "branch-target-enforcement" }
-
-; Declarations don't prevent setting BTI
-; ASM:	.word	3221225472
-; ASM-NEXT:	.word	4
-; ASM-NEXT:	.word	1
-
-; OBJ: Properties: aarch64 feature: BTI
Index: llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-7.ll
===
--- llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-7.ll
+++ /dev/null
@@ -1,23 +0,0 @@
-; RUN: llc -mtriple=aarch64-linux %s   -o - 2>&1 | \
-; RUN:   FileCheck %s --check-prefix=ASM
-; RUN: llc -mtriple=aarch64-linux %s -filetype=obj -o -  |  \
-; RUN:   llvm-readelf -S - | FileCheck %s --check-prefix=OBJ
-
-define dso_local i32 @f() #0 {
-entry:
-  ret i32 0
-}
-
-define dso_local i32 @g() #1 {
-entry:
-  ret i32 0
-}
-
-attributes #0 = { "sign-return-address"="non-leaf" }
-
-attributes #1 = { "branch-target-enforcement" }
-
-; No common attribute, no note section
-; ASM: warning: not setting BTI in feature flags
-; ASM-NOT: .note.gnu.property
-; OBJ-NOT: .note.gnu.property
Index: llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-6.ll
===
--- llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-6.ll
+++ /dev/null
@@ -1,22 +0,0 @@
-; RUN: llc -mtriple=aarch64-linux %s   -o - | \
-; RUN:   FileCheck %s --check-prefix=ASM
-; RUN: llc -mtriple=aarch64-linux %s -filetype=obj -o - |  \
-; RUN:   llvm-readelf -S - | FileCheck %s --check-prefix=OBJ
-
-define dso_local i32 @f() #0 {
-entry:
-  ret i32 0
-}
-
-define dso_local i32 @g() #1 {
-entry:
-  ret i32 0
-}
-
-attributes #0 = { "sign-return-address"="non-leaf" }
-
-attributes #1 = { "sign-return-address"="none" }
-
-; No common attribute, no note section
-; ASM-NOT: .note.gnu.property
-; OBJ-NOT: .note.gnu.property
Index: llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-5.ll
===
--- llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-5.ll
+++ /dev/null
@@ -1,26 +0,0 @@
-; RUN: llc -mtriple=aarch64-linux %s   -o - 2>&1 | \
-; RUN:   FileCheck %s --check-prefix=ASM
-; RUN: llc -mtriple=aarch64-linux %s -filetype=obj -o -  |  \
-; RUN:   llvm-readelf --notes - | FileCheck %s --check-prefix=OBJ
-
-define dso_local i32 @f() #0 {
-entry:
-  ret i32 0
-}
-
-define dso_local i32 @g() #1 {
-entry:
-  ret i32 0
-}
-
-attributes #0 = { "branch-target-enforcement" "sign-return-address"="non-leaf" }
-
-attributes #1 = { "sign-return-address"="all" }
-
-; Only the common atttribute (PAC)
-; ASM: warning: not setting BTI in feature flags
-; ASM:	.word	3221225472
-; ASM-NEXT:	.word	4
-; ASM-NEXT:	.word	2
-
-; OBJ: Properties: aarch64 feature: PAC
Index: llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-4.ll
===
--- llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-4.ll
+++ llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-4.ll
@@ -1,25 +1,12 @@
 ; RUN: llc -mtriple=aarch64-linux %s   -o - | \
 ; RUN:   FileCheck %s --check-prefix=ASM
-; RUN: llc -mtriple=aarch64-linux %s -filetype=obj -o - |  \
-; RUN:   llvm-readelf --notes - | FileCheck %s --check-prefix=OBJ
 
 define dso_local i32 @f() #0 {
 entry:
   ret i32 0
 }
 
-define dso_local i32 @g() #1 {
-entry:
-  ret i32 0
-}
-
 attributes #0 = { "branch-target-enforcement" "sign-return-address"="non-leaf" }
 
-attributes #1 = { "branch-target-enforcement" }
-
-; Only the common 

[PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

2020-08-21 Thread Momchil Velikov via Phabricator via cfe-commits
chill added a comment.

In D85649  I suggested a different version of 
module flags, which is a bit nicer to use, e.g. one can say just

  getModuleFlag("sign-return-address-with-bkey") != nullptr

instead of a) checking for the flag presence, b) getting its value and c) 
comparing it to a set of strings, which is
way too verbose.

Thus, the set of module flags are essentially booleans:

- "sign-return-address" when PAC-RET is enabled; it establishes the defaults of 
signing non-leaf functions with the A key
- "sign-return-address-all", modifies the default, established by 
"sign-return-address" to signing all functions, including ones that do not 
spill LR
- "sign-return-address-with-bkey", modifies the default, established by 
"sign-return-address" to signing with the B key.

These are not ABI, so if, in the future, if we do need a set of values, we can 
easily change it.


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

https://reviews.llvm.org/D80791

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


[PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

2020-08-11 Thread Szabolcs Nagy via Phabricator via cfe-commits
nsz added a comment.

In D80791#2210207 , @chill wrote:

> In D80791#2210124 , @danielkiss 
> wrote:
>
 it is not useful to have a bti annotated function unless everything else 
 is bti compatible too: it is all or nothing per elf module.
>>>
>>> This is false. Some functions in an elf module could be in a guarded 
>>> region, some in a non-guarded region. Some function may always
>>> be called in a "BTI-safe" way, which may be unknown to the compiler.
>>
>> Right now the elf and all of the `text` sections considered BTI enabled or 
>> not. The dynamic linkers/loaders can't support this
>> use case without additional information to be encoded somewhere (and 
>> specified). To support such we need to consider grouping/align to page
>> boundaries these functions in the linker because BTI could be controlled by 
>> flags in PTE.
>> With the current spec this usecase is not supported in this way. The user 
>> have to link the BTI protected code into another elf.
>> Side note: The `force-bti` linker option can't work with half BTI enabled 
>> objects.
>
> I suppose this is valid for typical Linux-based systems today.
>
> Is it valid in general, across the whole spectre of operating systems or for 
> bare-metal targets?
>
> Guess not.

the linker may or may not need to generate code (PLT is just one example) and 
the current abi is designed such that it is an elf module level decision if 
that code is bti compatible or not.

this is why i said that the current abi does not support mixed bti and non-bti 
code, however the user can do this if lies to the linker that everything is bti 
compatible so the linker generates stub code accordingly.

i don't see how baremetal can get away here: this is elf abi.


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

https://reviews.llvm.org/D80791

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


[PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

2020-08-11 Thread Momchil Velikov via Phabricator via cfe-commits
chill added a comment.

In D80791#2210124 , @danielkiss wrote:

>>> it is not useful to have a bti annotated function unless everything else is 
>>> bti compatible too: it is all or nothing per elf module.
>>
>> This is false. Some functions in an elf module could be in a guarded region, 
>> some in a non-guarded region. Some function may always
>> be called in a "BTI-safe" way, which may be unknown to the compiler.
>
> Right now the elf and all of the `text` sections considered BTI enabled or 
> not. The dynamic linkers/loaders can't support this
> use case without additional information to be encoded somewhere (and 
> specified). To support such we need to consider grouping/align to page
> boundaries these functions in the linker because BTI could be controlled by 
> flags in PTE.
> With the current spec this usecase is not supported in this way. The user 
> have to link the BTI protected code into another elf.
> Side note: The `force-bti` linker option can't work with half BTI enabled 
> objects.

I suppose this is valid for typical Linux-based systems today.

Is it valid in general, across the whole spectre of operating systems or for 
bare-metal targets?

Guess not.


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

https://reviews.llvm.org/D80791

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


[PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

2020-08-11 Thread Daniel Kiss via Phabricator via cfe-commits
danielkiss added a comment.

>> it is not useful to have a bti annotated function unless everything else is 
>> bti compatible too: it is all or nothing per elf module.
>
> This is false. Some functions in an elf module could be in a guarded region, 
> some in a non-guarded region. Some function may always
> be called in a "BTI-safe" way, which may be unknown to the compiler.

Right now the elf and all of the `text` sections considered BTI enabled or not. 
The dynamic linkers/loaders can't support this use case without additional 
information to be encoded somewhere (and specified). To support such we need to 
consider grouping/align to page boundaries these functions in the linker 
because BTI could be controlled by flags in PTE.
With the current spec this usecase is not supported in this way. The user have 
to link the BTI protected code into another elf.
Side note: The `force-bti` linker option can't work with half BTI enabled 
objects.


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

https://reviews.llvm.org/D80791

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


[PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

2020-08-11 Thread Szabolcs Nagy via Phabricator via cfe-commits
nsz added a comment.

In D80791#2209703 , @chill wrote:

> In D80791#2209624 , @nsz wrote:
>
>> - it is not useful to have a bti annotated function unless everything else 
>> is bti compatible too: it is all or nothing per elf module.
>
> This is false. Some functions in an elf module could be in a guarded region, 
> some in a non-guarded region. Some function may always
> be called in a "BTI-safe" way, which may be unknown to the compiler.

the current design is per elf module, so the non-guarded things have to go into 
a different elf module (and thus different tu).

i think the only way the current abi supports mixing bti and non-bti code is if 
all the linker inputs to the elf module are marked as bti compatible and then 
the user explicitly unprotects some region at runtime, i.e. bti is still all or 
nothing per elf module, but a user might want to do some hack and turn bti off 
in some places.

> With my proposal to derive marking from function attributes, as well as from 
> command-line
> everything above will still work in the (arguably) most common case that we 
> expect - users just using
> command line.
>
> I'm proposing to be strict and cover a few corner case where the command-line 
> only approach produces bogus results.

ok i think deriving the marking in the absence of command-line option works, 
but it's not something users can rely on and not what gcc does.


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

https://reviews.llvm.org/D80791

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


[PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

2020-08-11 Thread Momchil Velikov via Phabricator via cfe-commits
chill added a comment.

In D80791#2209624 , @nsz wrote:

> In D80791#2207203 , @chill wrote:
>
>> I would prefer to avoid the situation where the markings of two otherwise 
>> identical files were different,
>> depending on how the files were produced, no matter if it was a common or a 
>> special case.
>
> i don't see why it is desirable to silently get marking on an object file if 
> function definitions happen to be bti compatible in it:

It is desirable to get the marking because BTI-compatible functions don't 
appear by accident - they are a result of deliberate user actions, which clearly
express intent to use BTI.

> - compiler cannot reliably do this (e.g. bti incompatible inline asm).

Like for any other case, that's entirely the responsibility of the user if they 
use inline asm; Command-line options are not
special with regard to inline asm, so everything that can break 
attribute-derived marking, breaks command-line derived marking.

> - some users don't want the marking: not all linkers support it so it can 
> cause unexpected breakage.

Those linkers would need to be upgraded if the compiler imposes extra 
requirements on them.  One can't hold the compiler hostage to obsolete linkers. 
If users insist, they can just remove the .note section.

> - most users (all?) want the marking reliably (not opportunistically), but 
> function annotations are fragile (can depend on optimizations and code 
> outside of user control).

The user explicitly marking an object is the least reliable option, because 
it's done without regard what the object in actually contains.

> - it is not useful to have a bti annotated function unless everything else is 
> bti compatible too: it is all or nothing per elf module.

This is false. Some functions in an elf module could be in a guarded region, 
some in a non-guarded region. Some function may always
be called in a "BTI-safe" way, which may be unknown to the compiler.

> - but a compiler cannot diagnose if only some functions have the annotation 
> (we don't have a cflag for it) so even if the compiler tried to add the 
> marking silently users cannot rely on it: it's too easy to drop the marking 
> and no way to debug such failure.

At the time a compiler decides to or decides not to emit instructions which 
implement PAC-RET or BTI is perfectly clear what;s the effective annotation for 
each individual function.

I don't really understand the point of all these objections.

With my proposal to derive marking from function attributes, as well as from 
command-line
everything above will still work in the (arguably) most common case that we 
expect - users just using
command line.

I'm proposing to be strict and cover a few corner case where the command-line 
only approach produces bogus results.


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

https://reviews.llvm.org/D80791

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


[PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

2020-08-11 Thread Szabolcs Nagy via Phabricator via cfe-commits
nsz added a comment.

In D80791#2207203 , @chill wrote:

> I would prefer to avoid the situation where the markings of two otherwise 
> identical files were different,
> depending on how the files were produced, no matter if it was a common or a 
> special case.

i don't see why it is desirable to silently get marking on an object file if 
function definitions happen to be bti compatible in it:

- compiler cannot reliably do this (e.g. bti incompatible inline asm).
- some users don't want the marking: not all linkers support it so it can cause 
unexpected breakage.
- most users (all?) want the marking reliably (not opportunistically), but 
function annotations are fragile (can depend on optimizations and code outside 
of user control).
- it is not useful to have a bti annotated function unless everything else is 
bti compatible too: it is all or nothing per elf module.
- but a compiler cannot diagnose if only some functions have the annotation (we 
don't have a cflag for it) so even if the compiler tried to add the marking 
silently users cannot rely on it: it's too easy to drop the marking and no way 
to debug such failure.


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

https://reviews.llvm.org/D80791

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


[PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

2020-08-10 Thread Momchil Velikov via Phabricator via cfe-commits
chill added a comment.

I would prefer to avoid the situation where the markings of two otherwise 
identical files were different,
depending on how the files were produced, no matter if it was a common or a 
special case.


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

https://reviews.llvm.org/D80791

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


[PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

2020-08-10 Thread Szabolcs Nagy via Phabricator via cfe-commits
nsz added a comment.

In D80791#2206933 , @chill wrote:

> In D80791#2206853 , @nsz wrote:
>
>> i think that cannot work.
>>
>> the implementation is free to inject arbitrary code into
>> user code so if the user does not tell the implementation
>> that it wants the entire tu to be bti safe then non-bti
>> code can end up in there. (e.g. ctor of an instrumentation
>> that is not realated to any particular function with the
>> bti marking)
>
> Certainly, there are cases it won't work, but there are definitely
> cases where it *can* work. Whatever the implementation does
> should be a deterministic consequence of implementing the relevant
> language standards together with implementation-defined behaviour,
> command-line options and language extensions (e..g attributes).
>
> Certainly I don't expect C++ ctorts/dtors in C code and gcov or
> sanitiser calls if I haven't given relevant 
> `-fprofile-whatever`/`-fsanitize=whatever`
> options. In that sense, the implementation cannot do whatever
> it pleases, it is constrained to a range of behaviours one can reason about.

i think it's a bad idea to use function level
attributes to control what markings we attach to
translation units and i would prefer to only add
markings to object files when the compiler is
asked to do so per tu.

i dont want to see source code changes
to enable bti, that should be only needed
for some special case that's way out of
standard conform code.


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

https://reviews.llvm.org/D80791

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


[PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

2020-08-10 Thread Momchil Velikov via Phabricator via cfe-commits
chill added a comment.

In D80791#2206853 , @nsz wrote:

> i think that cannot work.
>
> the implementation is free to inject arbitrary code into
> user code so if the user does not tell the implementation
> that it wants the entire tu to be bti safe then non-bti
> code can end up in there. (e.g. ctor of an instrumentation
> that is not realated to any particular function with the
> bti marking)

Certainly, there are cases it won't work, but there are definitely
cases where it *can* work. Whatever the implementation does
should be a deterministic consequence of implementing the relevant
language standards together with implementation-defined behaviour,
command-line options and language extensions (e..g attributes).

Certainly I don't expect C++ ctorts/dtors in C code and gcov or
sanitiser calls if I haven't given relevant 
`-fprofile-whatever`/`-fsanitize=whatever`
options. In that sense, the implementation cannot do whatever
it pleases, it is constrained to a range of behaviours one can reason about.


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

https://reviews.llvm.org/D80791

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


Re: [PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

2020-08-10 Thread Szabolcs Nagy via cfe-commits
The 08/10/2020 10:03, Momchil Velikov via Phabricator wrote:
> chill added a comment.
> 
> In D80791#2196598 , @nsz wrote:
> 
> > the assumption is that the intended branch protection is implied via 
> > cmdline flags for the tu and function attributes are only used in source 
> > code for some hack.
> 
> I don't share this assumption. I find it just as valid to control the PAC/BTI 
> with things like:
> 
>   #ifdef ENABLE_BTI
>   #define BTI_FUNC __attribute__((target("branch-protection=bti")))
>   #else
>   #define BTI_FUNC
>   
>   BTI_FUNC void foo() { ...
>   BTI_FUNC int bar() { ...
> 
> without using any command-line option other than `-DENABLE_BTI=1`.
> 

i think that cannot work.

the implementation is free to inject arbitrary code into
user code so if the user does not tell the implementation
that it wants the entire tu to be bti safe then non-bti
code can end up in there. (e.g. ctor of an instrumentation
that is not realated to any particular function with the
bti marking)

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

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


[PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

2020-08-10 Thread Momchil Velikov via Phabricator via cfe-commits
chill added a comment.

In D80791#2196598 , @nsz wrote:

> the assumption is that the intended branch protection is implied via cmdline 
> flags for the tu and function attributes are only used in source code for 
> some hack.

I don't share this assumption. I find it just as valid to control the PAC/BTI 
with things like:

  #ifdef ENABLE_BTI
  #define BTI_FUNC __attribute__((target("branch-protection=bti")))
  #else
  #define BTI_FUNC
  
  BTI_FUNC void foo() { ...
  BTI_FUNC int bar() { ...

without using any command-line option other than `-DENABLE_BTI=1`.


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

https://reviews.llvm.org/D80791

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


[PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

2020-08-05 Thread Momchil Velikov via Phabricator via cfe-commits
chill added inline comments.



Comment at: clang/lib/CodeGen/TargetInfo.cpp:5550
+auto  = CGM.getLLVMContext();
+M->addModuleFlag(llvm::Module::Override, "sign-return-address",
+ Scope == LangOptions::SignReturnAddressScopeKind::All

Wouldn't that cause the sanitiser functions to be also compiled with PAC/BTI?  
(re: D75181)


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

https://reviews.llvm.org/D80791

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


[PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

2020-08-05 Thread Szabolcs Nagy via Phabricator via cfe-commits
nsz added a comment.

the gcc behaviour is not exactly ideal, but it's better if llvm is compatible 
with it or fix gcc if something is broken there.

the assumption is that the intended branch protection is implied via cmdline 
flags for the tu and function attributes are only used in source code for some 
hack. a common reason for such hack is to disable bti somewhere but still keep 
the bti elf marking. (if the intention was to mark the code non-bti compatible 
then just dont compile it with bti, using a non-portable  bti specfic function 
attribute would not work well for such use anyway)

now this may not work well with lto when functions can come from different 
places and the function attributes encode how those were compiled. so hopefully 
there is something that preserves the flags of the translation units and 
explicitly specified function attributes can be treated separately (such that 
they dont affect elf markings).


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

https://reviews.llvm.org/D80791

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


[PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

2020-08-05 Thread Daniel Kiss via Phabricator via cfe-commits
danielkiss updated this revision to Diff 283172.
danielkiss edited the summary of this revision.
danielkiss added a comment.

This version of the patch behaves as `gcc` for case when no function present 
and when function has `-mbranch-protection` attribute without compiler flag.
The logic should be in clang because in llvm we won't have enough information 
to handle these things. (see D75181 )


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

https://reviews.llvm.org/D80791

Files:
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/TargetInfo.cpp
  llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-0.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-1.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-2.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-3.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-4.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-5.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-6.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-7.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-8.ll

Index: llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-8.ll
===
--- llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-8.ll
+++ /dev/null
@@ -1,21 +0,0 @@
-; RUN: llc -mtriple=aarch64-linux %s   -o - | \
-; RUN:   FileCheck %s --check-prefix=ASM
-; RUN: llc -mtriple=aarch64-linux %s -filetype=obj -o - | \
-; RUN:   llvm-readelf --notes - | FileCheck %s --check-prefix=OBJ
-
-define dso_local i32 @f() #0 {
-entry:
-  %r = tail call i32 @g()
-  ret i32 %r
-}
-
-declare dso_local i32 @g()
-
-attributes #0 = { "branch-target-enforcement" }
-
-; Declarations don't prevent setting BTI
-; ASM:	.word	3221225472
-; ASM-NEXT:	.word	4
-; ASM-NEXT:	.word	1
-
-; OBJ: Properties: aarch64 feature: BTI
Index: llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-7.ll
===
--- llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-7.ll
+++ /dev/null
@@ -1,23 +0,0 @@
-; RUN: llc -mtriple=aarch64-linux %s   -o - 2>&1 | \
-; RUN:   FileCheck %s --check-prefix=ASM
-; RUN: llc -mtriple=aarch64-linux %s -filetype=obj -o -  |  \
-; RUN:   llvm-readelf -S - | FileCheck %s --check-prefix=OBJ
-
-define dso_local i32 @f() #0 {
-entry:
-  ret i32 0
-}
-
-define dso_local i32 @g() #1 {
-entry:
-  ret i32 0
-}
-
-attributes #0 = { "sign-return-address"="non-leaf" }
-
-attributes #1 = { "branch-target-enforcement" }
-
-; No common attribute, no note section
-; ASM: warning: not setting BTI in feature flags
-; ASM-NOT: .note.gnu.property
-; OBJ-NOT: .note.gnu.property
Index: llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-6.ll
===
--- llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-6.ll
+++ /dev/null
@@ -1,22 +0,0 @@
-; RUN: llc -mtriple=aarch64-linux %s   -o - | \
-; RUN:   FileCheck %s --check-prefix=ASM
-; RUN: llc -mtriple=aarch64-linux %s -filetype=obj -o - |  \
-; RUN:   llvm-readelf -S - | FileCheck %s --check-prefix=OBJ
-
-define dso_local i32 @f() #0 {
-entry:
-  ret i32 0
-}
-
-define dso_local i32 @g() #1 {
-entry:
-  ret i32 0
-}
-
-attributes #0 = { "sign-return-address"="non-leaf" }
-
-attributes #1 = { "sign-return-address"="none" }
-
-; No common attribute, no note section
-; ASM-NOT: .note.gnu.property
-; OBJ-NOT: .note.gnu.property
Index: llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-5.ll
===
--- llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-5.ll
+++ /dev/null
@@ -1,26 +0,0 @@
-; RUN: llc -mtriple=aarch64-linux %s   -o - 2>&1 | \
-; RUN:   FileCheck %s --check-prefix=ASM
-; RUN: llc -mtriple=aarch64-linux %s -filetype=obj -o -  |  \
-; RUN:   llvm-readelf --notes - | FileCheck %s --check-prefix=OBJ
-
-define dso_local i32 @f() #0 {
-entry:
-  ret i32 0
-}
-
-define dso_local i32 @g() #1 {
-entry:
-  ret i32 0
-}
-
-attributes #0 = { "branch-target-enforcement" "sign-return-address"="non-leaf" }
-
-attributes #1 = { "sign-return-address"="all" }
-
-; Only the common atttribute (PAC)
-; ASM: warning: not setting BTI in feature flags
-; ASM:	.word	3221225472
-; ASM-NEXT:	.word	4
-; ASM-NEXT:	.word	2
-
-; OBJ: Properties: aarch64 feature: PAC
Index: llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-4.ll
===
--- llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-4.ll
+++ llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-4.ll
@@ -1,25 +1,12 @@
 ; RUN: llc -mtriple=aarch64-linux %s   -o - | \
 ; RUN:   FileCheck %s --check-prefix=ASM
-; RUN: llc -mtriple=aarch64-linux %s -filetype=obj -o - |  \
-; RUN:   llvm-readelf --notes - | FileCheck %s 

[PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

2020-08-04 Thread Momchil Velikov via Phabricator via cfe-commits
chill added a comment.

In D80791#2164543 , @danielkiss wrote:

>> If any function has the attribute "sign-return-address", then the output note
>> section should have PAC bit set. The return address signing is completely 
>> local
>> to the function, and functions with or without return address signing can be
>> freely mixed with each other.
>
> That is true PAC and non-PAC functions can be mixed. 
> Does one function makes the "all executable sections" pac-ret enabled?

Yes, the presence of even a single function is a clear indication of what the 
user whats - enable PAC/BTI.
The default is not having PAC/BTI code gen, therefore its presence is a result 
of a deliberate action by the user, 
therefore unambiguously conveys the user's intent.

> BTW `GNU_PROPERTY_AARCH64_FEATURE_1_PAC` is not really used for anything.

I may not be used today in GNU/Linux, but still, it has to have sensible 
semantics.

> One of the reasons of the introduction of these macros is the management of 
> the function attributes.
> For example:
>
>   #ifdef __ARM_FEATURE_PAC_DEFAULT
>   #ifdef __ARM_FEATURE_BTI_DEFAULT
>   #define NO_PAC_FUNC __attribute__((target("branch-protection=bti")))
>   #else
>   #define NO_PAC_FUNC __attribute__((target("branch-protection=none")))
>   #endif /* __ARM_FEATURE_BTI_DEFAULT */
>   ...

I don't see how this example is relevant to the discussion of what notes to 
emit.
Our starting point is we have some default state (in module flags or whatever), 
some
individual function state and we have to decide what notes to emit, 
//regardless of the specific way
we came up with those function attributes.//

> In my humble opinion the function attribute is there to alter global setting.
> I considered to propagate the function attribute to the module flags but 
> that would lead to inconsistent compilation with the macros that I'd avoid.

The compilation of a single given function does not necessarily need to be
consistent with the value of these macros. Quite the opposite really, the 
macros themselves are
suffixed by `_DEFAULT` in order to explicitly acknowledge that possibility.

>> What do to if there are no functions in the compile unit?
>>
>> Technically, objects produced from such a unit are fully compatible with 
>> both PAC and BTI, which
>> means both flags should be set. But looking at the (non-existent) function 
>> attributes alone does
>> not allow us to unambiguously derive a user's intent to use PAC/BTI. In this 
>> case, I would suggest
>> setting the ELF note flags, according to the LLVM IR module flags.
>
> I think the only clear indication from the user to use PAC/BTI is the 
> explicit use of `-mbranch-protection=...` command-line option.

Using the attribute is no less clear and even carries more weight, as it 
overrides the command line option.

> A few function attributes that would turn PAC/BTI on just on those few 
> functions makes no sense for me in any real world application.

Turning on/off PAC/BTI is completely symmetrical - one can achieve exactly the 
same effect with:

- command-line options enabling PAC/BTI and individual attributes disabling BTI
- command-line options disabling PAC/BIT (e.g. not having a command-line option 
at all) and individual attributes enabling it

We shouldn't be guessing and prescribing how applications should use the 
mechanisms we make available and certainly
shouldn't be judging what is a real-world application and what is not.

> Valid to turn off PAC/BTI on selected functions while the whole application 
> compiled with them.
>
> We need to turn PAC off on the code path where we change\manage the keys for 
> example.
> Exaggerated example for BTI: https://godbolt.org/z/Y9bhe9  Current version of 
> llvm issues a warning and won't emit the note while I think it should.

Just as valid is to turn on PAC/BTI on selected functions, while the while 
compilation unit (*not* application) is compiled without them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80791

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


[PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

2020-07-21 Thread Daniel Kiss via Phabricator via cfe-commits
danielkiss added a comment.

> If any function has the attribute "sign-return-address", then the output note
>  section should have PAC bit set. The return address signing is completely 
> local
>  to the function, and functions with or without return address signing can be
>  freely mixed with each other.

That is true PAC and non-PAC functions can be mixed. 
Does one function makes the "all executable sections" pac-ret enabled?
BTW `GNU_PROPERTY_AARCH64_FEATURE_1_PAC` is not really used for anything.

> Likewise, if any function has the attribute "branch-target-enforcement", then
>  the output note section should have the BTI flag set. Even though code 
> compiled
>  with BTI is not necessarily compatible with non-BTI code:
> 
> - the only way to get BTI code is by explicit use of 
> `-mbranch-protection=...` command-line option, or the corresponding 
> attribute, which we should consider a clear indication about the user's 
> intent to use BTI.
> - the only way to get a mix of present/non-present 
> "branch-target-enforcement" attributes is by the explicit use of the 
> `__attribute__((target("branch-protection=..."))`, in which case we should 
> assume the user knows what they are doing.

`__ARM_FEATURE_PAC_DEFAULT` and `__ARM_FEATURE_BTI_DEFAULT` controlled by the 
`-mbranch-protection=...` 
https://developer.arm.com/documentation/101028/0011/Feature-test-macros?lang=en

One of the reasons of the introduction of these macros is the management of the 
function attributes.
For example:

  #ifdef __ARM_FEATURE_PAC_DEFAULT
  #ifdef __ARM_FEATURE_BTI_DEFAULT
  #define NO_PAC_FUNC __attribute__((target("branch-protection=bti")))
  #else
  #define NO_PAC_FUNC __attribute__((target("branch-protection=none")))
  #endif /* __ARM_FEATURE_BTI_DEFAULT */
  ...

In my humble opinion the function attribute is there to alter global setting.
I considered to propagate the function attribute to the module flags but 
that would lead to inconsistent compilation with the macros that I'd avoid.

> What do to if there are no functions in the compile unit?
> 
> Technically, objects produced from such a unit are fully compatible with both 
> PAC and BTI, which
>  means both flags should be set. But looking at the (non-existent) function 
> attributes alone does
>  not allow us to unambiguously derive a user's intent to use PAC/BTI. In this 
> case, I would suggest
>  setting the ELF note flags, according to the LLVM IR module flags.

I think the only clear indication from the user to use PAC/BTI is the explicit 
use of `-mbranch-protection=...` command-line option.
A few function attributes that would turn PAC/BTI on just on those few 
functions makes no sense for me in any real world application. 
Valid to turn off PAC/BTI on selected functions while the whole application 
compiled with them.

We need to turn PAC off on the code path where we change\manage the keys for 
example.
Exaggerated example for BTI: https://godbolt.org/z/Y9bhe9  Current version of 
llvm issues a warning and won't emit the note while I think it should.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80791



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


[PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

2020-07-21 Thread Momchil Velikov via Phabricator via cfe-commits
chill added a comment.

I don't think this behaviour is correct with regard to the specification 
(AAELF64 2020Q2):

> Static linkers processing ELF relocatable objects must set the feature bit in 
> the output object or image
>  only if all the input objects have the corresponding feature bit set.



> GNU_PROPERTY_AARCH64_FEATURE_1_BTI This indicates that all executable 
> sections are compatible with
>  Branch Target Identification mechanism. An executable or shared object with 
> this bit set is required to
>  generate Custom PLTs (page 35) with BTI instruction.
> 
> GNU_PROPERTY_AARCH64_FEATURE_1_PAC This indicates that all executable 
> sections have Return Address
>  Signing enabled. An executable or shared object with this bit set can 
> generate Custom PLTs (page 35)
>  with a PAC instruction.

Compatibility of executable sections ultimately depends on each individual 
function, therefore
it cannot be inferred from command-line options alone (transitively from module 
flags), which
merely set a default that can be overridden by function attributes.

If any function has the attribute "sign-return-address", then the output note
section should have PAC bit set. The return address signing is completely local
to the function, and functions with or without return address signing can be
freely mixed with each other.

Likewise, if any function has the attribute "branch-target-enforcement", then
the output note section should have the BTI flag set. Even though code compiled
with BTI is not necessarily compatible with non-BTI code:

- the only way to get BTI code is by explicit use of `-mbranch-protection=...` 
command-line option, or the corresponding attribute, which we should consider a 
clear indication about the user's intent to use BTI.
- the only way to get a mix of present/non-present "branch-target-enforcement" 
attributes is by the explicit use of the 
`__attribute__((target("branch-protection=..."))`, in which case we should 
assume the user knows what they are doing.

What do to if there are no functions in the compile unit?

Technically, objects produced from such a unit are fully compatible with both 
PAC and BTI, which
means both flags should be set. But looking at the (non-existent) function 
attributes alone does
not allow us to unambiguously derive a user's intent to use PAC/BTI. In this 
case, I would suggest
setting the ELF note flags, according to the LLVM IR module flags.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80791



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


[PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

2020-06-29 Thread Daniel Kiss via Phabricator via cfe-commits
danielkiss added a comment.

In D80791#2120755 , @kees wrote:

> In D80791#2120503 , @nickdesaulniers 
> wrote:
>
> > Might someone wish to disable PAC/BTI on an individual function, while 
> > having it on for the rest?  I guess that would mean you can't call that 
> > function indirectly?
>
>
> It would mean you can't call it _at all_, not just indirectly. :) Which is 
> why I still think the warning is useful. Perhaps don't warn for the functions 
> with the attribute?


BTI landing pad is needed only when the previous instructions was an indirect 
branch/jump  `bl x16`. 
With a direct branch `bl foo` no landing pad is needed at all. Rational is that 
direct branches can't be hijacked, they always land at the same location. 
Landing pads emitted only when indirect branch is possible to a function.[1]

[1] 
https://github.com/llvm/llvm-project/blob/f45b41348ba49c4a76baab1e3e302ef8e2bb992b/llvm/lib/Target/AArch64/AArch64BranchTargets.cpp#L94


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80791



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


[PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

2020-06-29 Thread Kees Cook via Phabricator via cfe-commits
kees added a comment.

In D80791#2120503 , @nickdesaulniers 
wrote:

> Might someone wish to disable PAC/BTI on an individual function, while having 
> it on for the rest?  I guess that would mean you can't call that function 
> indirectly?


It would mean you can't call it _at all_, not just indirectly. :) Which is why 
I still think the warning is useful. Perhaps don't warn for the functions with 
the attribute?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80791



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


[PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

2020-06-29 Thread Nick Desaulniers via Phabricator via cfe-commits
nickdesaulniers added a comment.

In D80791#2120090 , @kees wrote:

> Should the per-function analysis warning actually be removed? That seems like 
> a helpful check to catch a different form of bad behavior.


Might someone wish to disable PAC/BTI on an individual function, while having 
it on for the rest?  I guess that would mean you can't call that function 
indirectly?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80791



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


[PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

2020-06-29 Thread Daniel Kiss via Phabricator via cfe-commits
danielkiss added a comment.

Function level attributes could set different attributes for functions. If 
function attribute is used then I assume the user know what he/she is doing so 
no need to emit a warning.
Maybe some would ensure the function is only directly called and never called 
indirectly by enforcing the branch-proctection=None or Pac-ret.
By default this warning shall not present anyway, especially for LLVM internals.

> Specifically, this appears to be a legitimate bug, found by the warnings: 
> https://bugs.llvm.org/show_bug.cgi?id=46258

Thanks for pointing to this bug, I pick it up, since my two other patches 
address it:
https://reviews.llvm.org/D75181
and for CFI https://reviews.llvm.org/D81251

after those two patches, if we would emit a warning for the different flags we 
could do it in clang due to no reason to postpone the diagnostic to the 
backend. also would be easier suppress the warning if needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80791



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


[PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

2020-06-29 Thread Kees Cook via Phabricator via cfe-commits
kees added a comment.

Specifically, this appears to be a legitimate bug, found by the warnings: 
https://bugs.llvm.org/show_bug.cgi?id=46258


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80791



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


[PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

2020-06-29 Thread Kees Cook via Phabricator via cfe-commits
kees added a comment.

Should the per-function analysis warning actually be removed? That seems like a 
helpful check to catch a different form of bad behavior.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80791



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


[PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

2020-06-29 Thread Daniel Kiss via Phabricator via cfe-commits
danielkiss added a comment.

This patch solve this too: https://bugs.llvm.org/show_bug.cgi?id=46480


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80791



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


[PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

2020-05-29 Thread Daniel Kiss via Phabricator via cfe-commits
danielkiss created this revision.
danielkiss added reviewers: tamas.petz, chill.
Herald added subscribers: llvm-commits, cfe-commits, hiraditya, kristof.beyls.
Herald added projects: clang, LLVM.

Generate .note.gnu.property for PAC and BTI if the compile unit is
compiled with -mbranch-protection. 
It is valid use case to have a non-bti protected function therefore the note 
shouldn't be
removed in that case. In this case only the direct call is valid to the 
function.
This behaviour matches with gcc.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D80791

Files:
  clang/lib/CodeGen/CodeGenModule.cpp
  llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-0.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-1.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-2.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-3.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-4.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-5.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-6.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-7.ll
  llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-8.ll

Index: llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-8.ll
===
--- llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-8.ll
+++ /dev/null
@@ -1,21 +0,0 @@
-; RUN: llc -mtriple=aarch64-linux %s   -o - | \
-; RUN:   FileCheck %s --check-prefix=ASM
-; RUN: llc -mtriple=aarch64-linux %s -filetype=obj -o - | \
-; RUN:   llvm-readelf --notes | FileCheck %s --check-prefix=OBJ
-
-define dso_local i32 @f() #0 {
-entry:
-  %r = tail call i32 @g()
-  ret i32 %r
-}
-
-declare dso_local i32 @g()
-
-attributes #0 = { "branch-target-enforcement" }
-
-; Declarations don't prevent setting BTI
-; ASM:	.word	3221225472
-; ASM-NEXT:	.word	4
-; ASM-NEXT:	.word	1
-
-; OBJ: Properties: aarch64 feature: BTI
Index: llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-7.ll
===
--- llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-7.ll
+++ /dev/null
@@ -1,23 +0,0 @@
-; RUN: llc -mtriple=aarch64-linux %s   -o - 2>&1 | \
-; RUN:   FileCheck %s --check-prefix=ASM
-; RUN: llc -mtriple=aarch64-linux %s -filetype=obj -o -  |  \
-; RUN:   llvm-readelf -S | FileCheck %s --check-prefix=OBJ
-
-define dso_local i32 @f() #0 {
-entry:
-  ret i32 0
-}
-
-define dso_local i32 @g() #1 {
-entry:
-  ret i32 0
-}
-
-attributes #0 = { "sign-return-address"="non-leaf" }
-
-attributes #1 = { "branch-target-enforcement" }
-
-; No common attribute, no note section
-; ASM: warning: not setting BTI in feature flags
-; ASM-NOT: .note.gnu.property
-; OBJ-NOT: .note.gnu.property
Index: llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-6.ll
===
--- llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-6.ll
+++ /dev/null
@@ -1,22 +0,0 @@
-; RUN: llc -mtriple=aarch64-linux %s   -o - | \
-; RUN:   FileCheck %s --check-prefix=ASM
-; RUN: llc -mtriple=aarch64-linux %s -filetype=obj -o - |  \
-; RUN:   llvm-readelf -S | FileCheck %s --check-prefix=OBJ
-
-define dso_local i32 @f() #0 {
-entry:
-  ret i32 0
-}
-
-define dso_local i32 @g() #1 {
-entry:
-  ret i32 0
-}
-
-attributes #0 = { "sign-return-address"="non-leaf" }
-
-attributes #1 = { "sign-return-address"="none" }
-
-; No common attribute, no note section
-; ASM-NOT: .note.gnu.property
-; OBJ-NOT: .note.gnu.property
Index: llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-5.ll
===
--- llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-5.ll
+++ /dev/null
@@ -1,26 +0,0 @@
-; RUN: llc -mtriple=aarch64-linux %s   -o - 2>&1 | \
-; RUN:   FileCheck %s --check-prefix=ASM
-; RUN: llc -mtriple=aarch64-linux %s -filetype=obj -o -  |  \
-; RUN:   llvm-readelf --notes | FileCheck %s --check-prefix=OBJ
-
-define dso_local i32 @f() #0 {
-entry:
-  ret i32 0
-}
-
-define dso_local i32 @g() #1 {
-entry:
-  ret i32 0
-}
-
-attributes #0 = { "branch-target-enforcement" "sign-return-address"="non-leaf" }
-
-attributes #1 = { "sign-return-address"="all" }
-
-; Only the common atttribute (PAC)
-; ASM: warning: not setting BTI in feature flags
-; ASM:	.word	3221225472
-; ASM-NEXT:	.word	4
-; ASM-NEXT:	.word	2
-
-; OBJ: Properties: aarch64 feature: PAC
Index: llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-4.ll
===
--- llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-4.ll
+++ llvm/test/CodeGen/AArch64/note-gnu-property-pac-bti-4.ll
@@ -1,25 +1,12 @@
 ; RUN: llc -mtriple=aarch64-linux %s   -o - | \
 ; RUN:   FileCheck %s --check-prefix=ASM
-; RUN: llc -mtriple=aarch64-linux %s -filetype=obj -o - |  \
-; RUN:   llvm-readelf --notes | FileCheck %s