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

[Momchil Velikov via Phabricator via cfe-commits]

chill added a comment.

In D80791#2164543 <https://reviews.llvm.org/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