In D85649 I changed the module flags to be always present and have a zero/non-zero value. That's needed during LTO, if a flag is present in one module and absent in another,
no error is reported and the existing flags is used in the merged module, affecting the codegen for the module that did not initially have the flag.

Rebased.

In D75044#2292302, @danielkiss wrote:

@chill ping.

In D85649#2282286, @nickdesaulniers wrote:

Thanks @tejohnson for the feedback. Error sounds fine. The only other question I have is that a common pattern in the Linux kernel for progressive support of the ARM ISA extensions is to build isolated translation units that either use those features unconditionally but wont call into those translation units at runtime if the extensions are not present, or the kernel will live patch itself (ie. "alternatives" patching). Maybe @mrutland has thoughts; with BTI be runtime-patchable or otherwise enable-able at runtime. ie. one kernel image that runs on hardware both with and without BTI ISA extensions?

If the plan is to have isolated TUs built with BTI, then the "Error" policy sounds like this would be incompatible; but I don't think the point of BTI is to have some small portion of the code built with it. IIRC, doesn't BTI uses the no-op encoding space? So I guess one image could run on hardware regardless of actual BTI hardware support. So it wouldn't be patched in at runtime either. In that case, via kernel config I guess we'd build everything with BTI, and have build time errors for translation units that accidentally dropped KBUILD_CFLAGS, which happens a lot, but is fixable in kernel's build system sources.

The final piece of food for thought is ensuring that we can LTO C code built with BTI can link with out of line asm; Clang usually needs the assembler directives in place in the source being assembled to enable architectural extensions. If "Error" policy is used, can we still LTO C and asm sources? I assume we "should" be able to, but it might save trouble down the road to test that locally. Particularly, having C code that calls asm routines and the visibility of those routines during LTO has given us trouble in the past, when enabling CFI (control flow integrity; which I believe has some overlap with BTI but some subtle differences as well; I wouldn't call them perfect substitutes).

LTO related fixes - user Error combining behaviour, always emit the attributes with values 0 or 1
so they are present and their values can be checked for being the same during.

Some tests started failing: http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-ubuntu/builds/9071

It looks like the only value that makes sense is Error - any other policy (existing or not) would potentially lead to meaningfully different code generated with or without LTO.

Thank you for your comments. I'd like to have the same behaviour with or without LTO, which is apparently not the case with this patch.

Ping.

Ping

Ping,

LGTM.

Ping.

LGTM. It'd be nice if we could get someone non-Arm to have a look too. though.

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

Emit function-level LLVM IR attributes only when there's a GCC-style function attribute and be explicit with enabling/disabling.
Fixed an error where "none" in deferred to module attributes, instead of overriding them.

No, not yet.

In addition to the disabling of BTI in D81251, there's an issue that we explicitly disable BTI via branch-protection=none, the attribute would just be missing and we'll pick up the module attributes, which is not what we want.

Ping?

In D85649#2231236, @tellenbach wrote:

Does this conflict with D80791?

It does, we're using different module attributes.

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.

Thanks!

Ping?

Ping?

In D85649#2207922, @nickdesaulniers wrote:

Is there a test for no module attributes + function level attribute enabling pac/bti that tests that just that function gets the proper handling?

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.

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:

Well, it wasn't completely broken, just a little bit :D

Updated the patch to unconditionally include PAC stripping instructions, NOP-space ones to non-NON-space ones, depending on
whether v8.3-a (soo to be '+pa') feature.

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.

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)

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

In D75181#2193447, @danielkiss wrote:

I don't see any other alternative option, I'm open to any other idea.

My original idea was to pass options to LLVM. I'll come up with a patch in a day or two (if it works) and then we'll see.

I'm in favour of changing these patches to unconditionally emit XPACLRI (i.e. HINT #whatever).

This approach looks way too hackish to me with multiple opposing attributes ("sign-return-address" vs. "ignore-sign-return-address")
and some convoluted logic to resolve the contradiction.

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?

Is this patch needed anymore?

In D75044#2186973, @chill wrote:

Let's postpone this just for a little bit, to settle on an approach to depth > 0.

Let's postpone this just for a little bit, to settle on an approach to depth > 0.

LGTM, thanks!

LGTM. Thanks!

In D75044#2165496, @chill wrote:

The issue is that the definition of the instructions XPAC{D,I} is incorrect: it does not mention at all the operand to those insns.

Err, they do mention the operand, but only as an input one, it should be input/output.

I'm afraid the patch does not work yet. For example, when the following program

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

Needs a regression test. This patch and the dependent patch clash, better with a single patch.

Thanks!

PIng.

Yeah, I'd prefer that, but then I'm hesitant to change APIs.

Pretty straightforward, LGTM. I'd suggest rewording the title (presumably commit message summary) into something like "Do not coerce bfloat arguments and returns to integers", as we're obviously still lowering C and C++ to LLVM LR.§§

Updated to take into account liveness of registers. Whenever we blanket push registers, the live ones are ordinary operands,
the dead ones are Undef operands.