This is an archive of the discontinued LLVM Phabricator instance.

Extend the `uwtable` attribute with unwind table kind
ClosedPublic

Authored by chill on Nov 24 2021, 10:15 AM.

Details

Summary

We have the clang -cc1 command-line option -funwind-tables=1|2 and
the codegen option `VALUE_CODEGENOPT(UnwindTables, 2, 0) ///< Unwind
tables (1) or asynchronous unwind tables (2)`. However, this is
encoded in LLVM IR by the presence or the absence of the uwtable
attribute, i.e. we lose the information whether to generate want just
some unwind tables or asynchronous unwind tables.

Asynchronous unwind tables take more space in the runtime image, I'd
estimate something like 80-90% more, as the difference is adding
roughly the same number of CFI directives as for prologues, only a bit
simpler (e.g. .cfi_offset reg, off vs. .cfi_restore reg). Or even
more, if you consider tail duplication of epilogue blocks.
Asynchronous unwind tables could also restrict code generation to
having only a finite number of frame pointer adjustments (an example
of *not* having a finite number of SP adjustments is on AArch64 when
untagging the stack (MTE) in some cases the compiler can modify SP
in a loop).
Having the CFI precise up to an instruction generally also means one
cannot bundle together CFI instructions once the prologue is done,
they need to be interspersed with ordinary instructions, which means
extra DW_CFA_advance_loc commands, further increasing the unwind
tables size.

That is to say, async unwind tables impose a non-negligible overhead,
yet for the most common use cases (like C++ exceptions), they are not
even needed.

This patch extends the uwtable attribute with an optional
value:

  • uwtable (default to async)
  • uwtable(sync), synchronous unwind tables
  • uwtable(async), asynchronous (instruction precise) unwind tables

Diff Detail

Event Timeline

chill created this revision.Nov 24 2021, 10:15 AM
chill requested review of this revision.Nov 24 2021, 10:15 AM
Herald added a reviewer: sstefan1. · View Herald Transcript
Herald added a reviewer: baziotis. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
chill updated this revision to Diff 393411.Dec 10 2021, 2:19 AM
chill updated this revision to Diff 399003.Jan 11 2022, 10:14 AM
chill added a comment.Jan 26 2022, 9:15 AM

Ping (the whole stack :D )?

I'd like to see names, not numbers, in printed LLVM IR attributes. It's more clear what it's supposed to mean, and it's easier to adjust in the future.

Did you mean to update https://llvm.org/docs/LangRef.html#synthesized-functions-module-flags-metadata ?

Sorry for the belated response.

I support refining uwtable to two values.

I'd like to see names, not numbers, in printed LLVM IR attributes. It's more clear what it's supposed to mean, and it's easier to adjust in the future.

Do you have a suggestion for the sync unwind tables attribute name? syncuwtable?

Did you mean to update https://llvm.org/docs/LangRef.html#synthesized-functions-module-flags-metadata ?

Then “uwtable”: Max. The value can be 0 or 1. If the value is 1, a synthesized function will get the uwtable function attribute. will need note that syncuwtable is part of the uwtable family.

llvm/lib/IR/Attributes.cpp
211

delete excessive blank line

444

Result += ('(' + Twine(unsigned(kind)) + ')').str();

llvm/test/Bitcode/attributes.ll
520

Place { in the prevailing position

MaskRay added inline comments.Jan 26 2022, 10:42 AM
clang/test/CodeGen/uwtable-attr.c
20

It's worth describing the full shape so that it is clearer which IR part the CHECK lines are describing.

attributes #[[#]] = { {{.*}} uwtable{{ }}
![[#]] = !{{i32 7, !"uwtable", i32 1}
chill added a comment.Jan 27 2022, 4:30 AM

I'd like to see names, not numbers, in printed LLVM IR attributes. It's more clear what it's supposed to mean, and it's easier to adjust in the future.

I support refining uwtable to two values.

Do you have a suggestion for the sync unwind tables attribute name? syncuwtable?

By "two values", do you mean two separate attributes? Would you like to discuss this idea in this thread https://lists.llvm.org/pipermail/llvm-dev/2021-November/153768.html ?

chill updated this revision to Diff 404008.Jan 28 2022, 6:47 AM
chill marked 3 inline comments as done.
chill added inline comments.
llvm/lib/IR/Attributes.cpp
444

That's cool, but on top of D118451

By "two values", do you mean two separate attributes? Would you like to discuss this idea in this thread https://lists.llvm.org/pipermail/llvm-dev/2021-November/153768.html ?

I don't really have a preference about uwtable_asynchronous vs. uwtable((asynchronous)) or whatever. I just don't want the literal numbers "1" and "2" in IR.

chill added a comment.Jan 31 2022, 1:22 AM

By "two values", do you mean two separate attributes? Would you like to discuss this idea in this thread https://lists.llvm.org/pipermail/llvm-dev/2021-November/153768.html ?

I don't really have a preference about uwtable_asynchronous vs. uwtable((asynchronous)) or whatever. I just don't want the literal numbers "1" and "2" in IR.

Yep, I think I'm going with uwtable, uwtable(sync), and uwtable(async).

chill updated this revision to Diff 404583.Jan 31 2022, 9:53 AM
chill edited the summary of this revision. (Show Details)
chill marked an inline comment as done.Jan 31 2022, 9:54 AM
ormris removed a subscriber: ormris.Feb 1 2022, 9:53 AM
MaskRay accepted this revision.Feb 1 2022, 5:34 PM

(Your patch was not uploaded via arc diff, therefore arc patch D114543 has some merge conflicts to me.
I could still apply it with curl -L 'https://reviews.llvm.org/D114543?download=1' | patch -p0, though.)

Thanks for the update. LGTM.
There are some clang-format lints that should be fixed first.

clang/test/CodeGen/uwtable-attr.c
10

The comment applying to the whole file is generally placed at the top, before all RUN lines.

llvm/include/llvm/Support/CodeGen.h
74

See the clang-format lint

llvm/lib/AsmParser/LLParser.cpp
2017

This diagnostic needs a test in test/Assembler/

2019

The pattern can be simplified with parseToken.

llvm/lib/Bitcode/Reader/BitcodeReader.cpp
1656

clang-format lint

llvm/lib/IR/Attributes.cpp
443

clang-format wraps the continuation lines in a strange way. Perhaps switch to this form:

if (Kind == UWTableKind::Default)
  return "uwtable";
return ...
This revision is now accepted and ready to land.Feb 1 2022, 5:34 PM
chill updated this revision to Diff 407984.Feb 11 2022, 12:17 PM
chill marked 6 inline comments as done.
chill added inline comments.
llvm/lib/IR/Attributes.cpp
443

Yeah, that was my first variant

if (Kind != UWTableKind::None) {
  if (Kind == UWTableKind::Default)
    return "uwtable";
  return ("uwtable(" + Twine(Kind == UWTableKind::Sync ? "sync" : "async") +
          ")")
      .str();
}

but it looks slightly worse, IMHO.

MaskRay accepted this revision.Feb 11 2022, 3:03 PM
This revision was landed with ongoing or failed builds.Feb 14 2022, 6:35 AM
This revision was automatically updated to reflect the committed changes.
chill marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 14 2022, 6:35 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

As far as I can tell this patch broke the Rust compiler, but from the commit message it sounds like it shouldn't have?

https://buildkite.com/llvm-project/rust-llvm-integrate-prototype/builds/8358#e85ad6f3-9992-4ea1-9cd3-d8db9f45f33e fails with

Attribute 'uwtable' should have an Argument
i8* (i64, i64)* @__rust_alloc
in function __rust_alloc
LLVM ERROR: Broken function found, compilation aborted!

Any thoughts?

chill added a comment.Feb 14 2022, 9:17 AM

As far as I can tell this patch broke the Rust compiler, but from the commit message it sounds like it shouldn't have?

https://buildkite.com/llvm-project/rust-llvm-integrate-prototype/builds/8358#e85ad6f3-9992-4ea1-9cd3-d8db9f45f33e fails with

Attribute 'uwtable' should have an Argument
i8* (i64, i64)* @__rust_alloc
in function __rust_alloc
LLVM ERROR: Broken function found, compilation aborted!

Any thoughts?

Yeah, that's puzzling. The attribute has an optional argument (or else we had to update ~3080 occurrences of "uwtable" in tests), so reading it is
a bit tricky: https://github.com/llvm/llvm-project/blob/19b4e9d76ecc9a5343c093bc54d965734b996518/llvm/lib/Bitcode/Reader/BitcodeReader.cpp#L1631
That message is output here https://github.com/llvm/llvm-project/blob/19b4e9d76ecc9a5343c093bc54d965734b996518/llvm/lib/IR/Verifier.cpp#L1710
and I can trigger this line with

$ cat x.ll
define void @f() uwtable {
	ret void
}
$ ./bin/opt -S --passes=verify x.ll
; ModuleID = 'x.ll'
source_filename = "x.ll"

; Function Attrs: uwtable
define void @f() #0 {
  ret void
}

attributes #0 = { uwtable }
$ ./bin/opt  x.ll -o x.bc
$ ./bin/opt --verify  x.bc -S
; ModuleID = 'x.bc'
source_filename = "x.ll"

; Function Attrs: uwtable
define void @f() #0 {
  ret void
}

attributes #0 = { uwtable }
$ 

Could there be a mismatch between two llvm-project versions, somehow?

As far as I can tell this patch broke the Rust compiler, but from the commit message it sounds like it shouldn't have?

https://buildkite.com/llvm-project/rust-llvm-integrate-prototype/builds/8358#e85ad6f3-9992-4ea1-9cd3-d8db9f45f33e fails with

Attribute 'uwtable' should have an Argument
i8* (i64, i64)* @__rust_alloc
in function __rust_alloc
LLVM ERROR: Broken function found, compilation aborted!

Any thoughts?

Yeah, that's puzzling. The attribute has an optional argument (or else we had to update ~3080 occurrences of "uwtable" in tests), so reading it is
a bit tricky: https://github.com/llvm/llvm-project/blob/19b4e9d76ecc9a5343c093bc54d965734b996518/llvm/lib/Bitcode/Reader/BitcodeReader.cpp#L1631
That message is output here https://github.com/llvm/llvm-project/blob/19b4e9d76ecc9a5343c093bc54d965734b996518/llvm/lib/IR/Verifier.cpp#L1710
and I can trigger this line with

$ cat x.ll
define void @f() uwtable {
	ret void
}
$ ./bin/opt -S --passes=verify x.ll
; ModuleID = 'x.ll'
source_filename = "x.ll"

; Function Attrs: uwtable
define void @f() #0 {
  ret void
}

attributes #0 = { uwtable }
$ ./bin/opt  x.ll -o x.bc
$ ./bin/opt --verify  x.bc -S
; ModuleID = 'x.bc'
source_filename = "x.ll"

; Function Attrs: uwtable
define void @f() #0 {
  ret void
}

attributes #0 = { uwtable }
$ 

Could there be a mismatch between two llvm-project versions, somehow?

I don't believe so: we build LLVM from HEAD and build Rust directly against LLVM. Is the parameter optional if uwtable is set programmatically, or only when we're reading IR streams?

chill added a comment.Feb 14 2022, 9:35 AM

Is the parameter optional if uwtable is set programmatically, or only when we're reading IR streams?

No, it's not optional, the attribute is added by https://github.com/llvm/llvm-project/blob/00cd6c04202acf71f74c670b2dd4343929d1f45f/llvm/include/llvm/IR/Function.h#L636
(although seting it to None is semantically as not setting it at all).

Is the parameter optional if uwtable is set programmatically, or only when we're reading IR streams?

No, it's not optional, the attribute is added by https://github.com/llvm/llvm-project/blob/00cd6c04202acf71f74c670b2dd4343929d1f45f/llvm/include/llvm/IR/Function.h#L636
(although seting it to None is semantically as not setting it at all).

Okay, that makes sense then. I think I was close to getting to that point by looking at this patch, but you saved me some time. Thanks!

/me off to write matching patch for Rust

RKSimon added inline comments.
llvm/lib/IR/Attributes.cpp
453

@chill Static analysis is warning that its impossible to hit the if(Kind == Default) case here - it looks like you have merged 2 versions of the same (Kind != UWTableKind::None) handling code?

chill added inline comments.Feb 17 2022, 2:45 AM
llvm/lib/IR/Attributes.cpp
453

Thanks, indeed.

Fixed in https://reviews.llvm.org/D120030

chill marked an inline comment as done.Feb 17 2022, 4:25 AM