[MC] Add 'G' to augmentation string for MTE instrumented functions

[fmayer]

Summary

This was agreed on in
https://lists.llvm.org/pipermail/llvm-dev/2020-May/141345.html

The thread proposed two options

• add a character to augmentation string and handle in libuwind
• use a separate personality function.

It was determined that this is the simpler and better option.

This is part of ARM's Aarch64 ABI:
https://github.com/ARM-software/abi-aa/blob/main/aadwarf64/aadwarf64.rst#id22

The next step after this is teaching libunwind to untag when this
augmentation character is set.

[fmayer]

naming

[fmayer]

respect nounwind

[fmayer]

comment

[fmayer]

comment

[fmayer]

test

[MaskRay]

The lld/ELF change should be in a separate patch.
It's the convention and I am usually picky on how tests should be made.

[MaskRay]

This was agreed on in https://lists.llvm.org/pipermail/llvm-dev/2020-May/141345.html.

This agreement is insufficient for the broad Linux community.

Linux Standard Base Core Specification, Generic Part defines augmentation string characters. Essentially this needs some GCC agreement, otherwise there could be potential future conflict.

[fmayer]

In D127007#3557500, @MaskRay wrote:

This was agreed on in https://lists.llvm.org/pipermail/llvm-dev/2020-May/141345.html.

This agreement is insufficient for the broad Linux community.

Linux Standard Base Core Specification, Generic Part defines augmentation string characters. Essentially this needs some GCC agreement, otherwise there could be potential future conflict.

Thanks! I was wondering about this. I will get broader agreement prior to moving forward with this.

[fmayer]

Reached out to GCC here: https://gcc.gnu.org/pipermail/gcc/2022-June/238813.html

[fmayer]

split off lld / dwarf parser change.

[MaskRay]

Thanks for starting the GCC thread! I think the implementation is reasonable.

[llvm] Add 'G' to augmentation string for MTE instrumented functions.

[MC] may be more specific. The convention is to avoid trailing dot in the subject line.

This was agreed on in https://lists.llvm.org/pipermail/llvm-dev/2020-May/141345.html.

Avoid trailing . in an URI. Many tools consider . part of the URI.

Try summarizing the main point of the thread and place it in the commit message (summary).

llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp

1294

There are too many blank lines.

Use

if (getFunctionCFISectionType(*MF) != CFISection::None)
  OutStreamer->emitCFIMTETaggedFrame();

llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp

6527

if (parseEOL())
  return true;

I've updated other unexpected *** in ... directive to use the prevailing style expected newline.

llvm/test/CodeGen/AArch64/stack-tagging-cfi.ll

4

i32* => ptr

8

[fmayer]

In D127007#3559248, @MaskRay wrote:

Thanks for starting the GCC thread! I think the implementation is reasonable.

Turns out this is already part of ARM's ABI:
https://github.com/ARM-software/abi-aa/blob/main/aadwarf64/aadwarf64.rst#id22

[fmayer]

address comments

[MaskRay]

Looks great! Hope that @eugenis can check the test interop with msan.

[fmayer]

format

[eugenis]

Should we have a test for the augmentation string, through llvm-readelf?

llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp

1289

As we've seen recently, sometimes even nounwind frames get unwound. Is it possible to relax some of these conditions to emit the "G" in as many cases as possible? This is the situation where MTE is more demanding to the unwind correctness, so I'd rather err on the safe side.

[fmayer]

In D127007#3561986, @eugenis wrote:

Should we have a test for the augmentation string, through llvm-readelf?

We have that test the follow-up CL that does the change to lld (as requested to be split by MaskRay).

[fmayer]

llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp

1289

I'm not sure. This is exactly the same logic as for the B-Key, so either we change both or neither. B-Key is just as important to correctness as this is.

What do you mean "nounwind frames get unwound"? AFAICT they do get skipped over, so the 'G' wouldn't help.

[eugenis]

In D127007#3562002, @fmayer wrote:

In D127007#3561986, @eugenis wrote:

Should we have a test for the augmentation string, through llvm-readelf?

We have that test the follow-up CL that does the change to lld (as requested to be split by MaskRay).

Hmm ok. This tests does not really need to involve lld at all, but that's fine.

[fmayer]

In D127007#3562017, @eugenis wrote:

In D127007#3562002, @fmayer wrote:

In D127007#3561986, @eugenis wrote:

Should we have a test for the augmentation string, through llvm-readelf?

We have that test the follow-up CL that does the change to lld (as requested to be split by MaskRay).

Hmm ok. This tests does not really need to involve lld at all, but that's fine.

It does, because we need LLD so we can get an ELF file (which we then can readelf or, as the test does objdump).

[eugenis]

You can llvm-objdump --dwarf=frames a .o file.

[fmayer]

In D127007#3562022, @fmayer wrote:

In D127007#3562017, @eugenis wrote:

In D127007#3562002, @fmayer wrote:

In D127007#3561986, @eugenis wrote:

Should we have a test for the augmentation string, through llvm-readelf?

We have that test the follow-up CL that does the change to lld (as requested to be split by MaskRay).

Hmm ok. This tests does not really need to involve lld at all, but that's fine.

It does, because we need LLD so we can get an ELF file (which we then can readelf or, as the test does objdump).

Good point. The change to the DWARF parser for that is also in the follow up change though, but I changed the test to both check the .o and the .so files.

[fmayer]

llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp

1289

I think the only thing we can do is force unwind tables for all frames that have stack instrumentation, but that would be quite a big hammer.

I'm not sure that is the right thing to do, where the real solution would be for people to fix their builds to not mix exception and noexception code in this way. Given that this mode is (at least for now) intended for testing, to me that seems like the better thing for people to do, as all sorts of stuff can go wrong when throwing exceptions through no-exception code (PAC as mentioned above, anything that needs RAII, &c).

[MaskRay]

It may be better to move the DebugInfo change from the lld patch into this patch.

[fmayer]

In D127007#3562088, @MaskRay wrote:

It may be better to move the DebugInfo change from the lld patch into this patch.

I split it into D127171. Can also merge into here, no strong opinions

[fmayer]

add another test

llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp

1289

OK I think we have resolved this confusion. Compiling with -fno-exceptions does still generate IR with uwtables but also with`nounwind`. In this case the uwtable does still get generated, but not the cleanup actions. Added a test to that effect.

[eugenis]

LGTM