In D94083#2486800, @fhahn wrote:

FWIW I think it would be good to have a bit more details in the description for changes such as this, like a link to the public docs for the extension.

LGTM

LGTM.

FYI: https://bugs.llvm.org/show_bug.cgi?id=48472

In D91994#2422228, @chill wrote:

In the Arm Architecture Reference Manual Supplement - Armv8, for Armv8-R AArch64 architecture profile
"A1.4 Architecture extensions" lists among others

• FEAT_SHA1 Advanced SIMD SHA1 instructions
• FEAT_SHA256 Advanced SIMD SHA256 instructions
• FEAT_AES Advanced SIMD AES instructions
• FEAT_PMULL Advanced SIMD PMULL instructions

Is there a document, which says R82 does not implement those ?

In the Arm Architecture Reference Manual Supplement - Armv8, for Armv8-R AArch64 architecture profile
"A1.4 Architecture extensions" lists among others

LGTM, thanks.

Thanks!

Cheers!

Cheers!

That should be the minimum number of instructions now, one less and the outlining does not pass the cost threshold.

Ping.

In D89625#2367785, @yroux wrote:

I'd check if there are some calls of modifiesRegister that pass nullptr, the patch LGTM otherwise

Ping.

Ping.

Ping.

Ping.

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.