LGTM

In D127812#3602688, @aaron.ballman wrote:

In D127812#3602645, @erichkeane wrote:

In D127812#3601476, @danielkiss wrote:

Your understanding is correct. target attribute has two usage model. One is just redefine the to be used codegen options, this is used already widely for Arm and AArch64. The other use of the target attribute is the multi versioning and the rational for the target_version attribute is the easier distinction between the two usage mode, also not to break any code out there by changing the behaviour of an attribute.

I don't think differentiating the uses here is a good idea. I think it would have been a GREAT idea about 10 years ago, but that ship has already sailed once GCC started using it that way however. We should be keeping the current behavior, otherwise we're going to have a horrible mix of target/target_version working inconsistently between platforms.

That largely is my concern as well. The existing behavior of target is just that -- the existing behavior. I think deviating from that existing behavior will be confusing in practice. Adding additional attributes doesn't improve that confusion because users then have to know to decide between two very similar attributes, which means they then need to educate themselves on the differences between them. If we're going to push them towards the documentation to correctly use the attribute anyway, that's basically the same situation they're in today with the confusing dual behavior of target.

LGTM.

In D127812#3599530, @aaron.ballman wrote:

In D127812#3587223, @ilinpv wrote:

In D127812#3585249, @erichkeane wrote:

I'm concerned as to the design of this addition, I don't particularly appreciate the reasons for making 'target_clones' different, nor the purpose for adding a new attribute instead of using 'target' for what seems like exactly that? IF the new spelling is THAT necessary, we perhaps don't need a whole new attribute for it either.

Thank you for fair concern, "target_clones" for AArch64 has different format, semantic, e.g. "default" is not required. Therefore it diverges with X86 in these parts.

Is it *necessary* that it diverges like this? (Is there some published standards document you're trying to conform to, is there an implementation difficulty with not diverging, something else?)

Patch is only needed due to a clang bug which is fixed already in 11.x releases and TOT. For me the original code looks better and I don't see the need for the patch but I let other from the @libunwind group to disagree with me.

In D123493#3569025, @MaskRay wrote:

This behavior requires that all participating modules have the module flag. In the absence of the module flag, what should be behavior be?

For module flags like "sign-return-address", I see that you just emit the flag unconditionally (which can be zero).

Absence of the "branch-target-enforcement" and "sign-return-address"` means features are off. So the flag merger will take an existing flag and the feature could be turned on, which is bad.
AutoUpdater should insert the attributes with zero value if they are missing.

In D123692#3527595, @ldionne wrote:

c218fd3d7d3764eb123c8429bbcd33bacfe2e633 added libcxxabi/test/native/AArch64/ra_sign_state.pass.cpp -- was that intended, or should it have been added to libunwind/test/[...]? It also started failing in the CI in the -fno-exceptions configuration -- I think it is just missing a XFAIL so I'll add it, but please investigate whether the file needs to be moved (I think it does).

Tests in libunwind can't depend on libcxx (failed in llvm-libunwind-static.cfg.in) so the test is moved to libcxxabi, because test test case needs exception. (simple backtrace won't trigger the problem)

In D123692#3512996, @vvereschaka wrote:

Hi @danielkiss,

the FAIL: llvm-libunwind-static.cfg.in:: aarch64.ra_sign_state.pass.cpp test is getting failed on Linux Ubuntu/Aarch64 toolchain builders with the following output:

terminate called after throwing an instance of 'int'
terminate called recursively

here is more details: https://lab.llvm.org/buildbot/#/builders/119/builds/8501/steps/12/logs/FAIL__llvm-libunwind-static_cfg_in___aarch64_ra_si

https://lab.llvm.org/buildbot/#/builders/119/builds/8501

@MaskRay thanks so much.

ping?

some tests from libcxxabi could be enabled for SystemZ too e.g. libcxxabi/test/forced*

In D111780#3465221, @ardb wrote:

Can this change be taken into consideration again now that D114545 has been merged?

Yes, I'm running a few tests, will land later today.

I have looked at AArch64/Arm tests.
These are okay due to just Codegen is improved here:
llvm/test/CodeGen/AArch64/framelayout-sve.mir
llvm/test/CodeGen/AArch64/strict-fp-int-promote.ll
llvm/test/CodeGen/AArch64/sve-calling-convention-mixed.ll
llvm/test/CodeGen/AArch64/sve-ld1r.mir
llvm/test/CodeGen/AArch64/sve-ldstnt1.mir
llvm/test/CodeGen/ARM/arm-shrink-wrapping.ll
llvm/test/CodeGen/ARM/reg_sequence.ll
llvm/test/CodeGen/ARM/machine-outliner-calls.mir
llvm/test/CodeGen/ARM/jump-table-islands.ll
llvm/test/CodeGen/ARM/ifcvt-branch-weight-bug.ll
llvm/test/CodeGen/Thumb2/mve-vst4.ll
llvm/test/CodeGen/Thumb2/mve-fpclamptosat_vec.ll

Thanks, test is added.

In D123493#3443152, @pcc wrote:

Can we add logic to llvm/lib/IR/AutoUpgrade.cpp to change Error to Min if we see one of these module flags? I think that should allow old bitcode to continue working.

In D123493#3442303, @tschuett wrote:

This would silently reduce protection, when incompatible modules are linked?

Yes, but that happens also with the normal linker too by default.

Thanks for the patch.

Add to the commit message Fixes #48888 to make the connection on github.

LGTM, thanks for the patch.

LGMT

LGTM

Could you please rebase the series? it generated a lot of conflict for me.
the generated directives in the tests are looking good.

LGTM

Thanks for update.

This causes unwind problems when landing pads of the exception handlers involved, because for those the .cfi_negate_ra_state is processed twice and the signature from the return address is not stripped but should be.
Let's reland after D114545.

In D113901#3190060, @MaskRay wrote:

Out of town for ~10 days. Sorry for the long delay.
LGTM.

Not a problem, thanks!

In D103626#2824083, @DavidSpickett wrote:

Maybe this is something worth considering for LLDB memory dumps?

I'm working on that at the moment, it's on my github branch. This is what it looks like with the right options: (others look weird at the moment due to ordering issues)

(lldb) memory read mte_buf mte_buf+32 -f "x" -l 1 -s 16
0xfffff7ff6000: 0x00000000000000000000000000000000 (tag: 0x0)
0xfffff7ff6010: 0x00000000000000000000000000000000 (tag: 0x1)

The format is mostly a guess on my part, given that I'm mostly testing the debugger itself not MTE enabled software.

That looks really good, could help a lot to understand tagging related issues. Looking forward for that patch :)

ping?

Thanks for the rebase, LGTM

In D115140#3176800, @stuij wrote:

@danielkiss Yes that needs to be addressed, but we are doing that in another patch that will (hopefully) cover all permutations of architecture and branch protection values on both the cmdline and as function attributes.

sound good, Thanks!

I think rejecting\warning the b-key from the command line maybe a more developer friendly way.

Thanks for the update. LGTM.

fixed in other changes.

Cloud you please rebase this patch? there were a few changes to __cxa_end_cleanup recently.

In D114818#3162689, @ldionne wrote:

Do we need to apply __attribute__((noinline)) to main? Can you try that out and let me know if that helps?

Clang automatically adds noinline to main.

LGTM besides NITs.
Could you make clang-format happy?

In D109856#3154763, @mstorsjo wrote:

Looks reasonable I think. Is this a deficiency in the EHABI implementation only, i.e. this aspect works as it should in the regular dwarf implementation?

Yes, it works on dwarf already ( also the test pass on X86/Aarch64 ).

Hopefully later other reports like lam-report can be added easier.

Ping

In D113901#3137065, @peter.smith wrote:

In D113901#3135319, @MaskRay wrote:

-z force-bti with -Werror has the same behaviour but not always possible to use -Werror and dangerous to rely on it.

GCC/Clang support -Werror. ld.bfd/gold/ld.lld support --fatal-warnings.

In D113901#3134421, @peter.smith wrote:

I'm in favour of adding the additional flag. It is useful when an effort has been made to get all input files marked, and we want to make sure that the program stays that way. I think that complements the existing force-bti flag which is useful when transitioning a program into compatibility.

One thought is that this could also be useful for ibt. Is there scope for -z fail-if-not=bti and -z fail-if-not=ibt which might make this more extensible for future property flags?

Does this have the same semantics as -z cet-report=error? (https://bugs.llvm.org/show_bug.cgi?id=45483)
If yes, we can use a similar tri-state option.

It does look very similar, the -z cet-report=error also includes a warning state, whereas -z fail-if-not=bti only has an error state. Will have to let Daniel comment on that.

LGTM

I think we can drop this change.

LGTM, Thanks.

LGTM, any triplet could work here, where it matters we can add a new RUN line with an other triplet.

LGTM, I'm happy with the current state.

In D51650#3121789, @erichkeane wrote:

But nit made.

NIT: clang-format issues still present. Maybe you need to update your local clang-format.

Make the CI happy.