Page MenuHomePhabricator

[MS][NFC] Remove SEH chaininfo assembler macros
Needs ReviewPublic

Authored by namazso on Dec 16 2021, 10:30 PM.

Details

Reviewers
rnk
Summary

These macros cannot be used to generate valid output and it has been like that since their introduction 10 years ago.

Chaining is to be used for function chunks out of the main function's region, and RUNTIME_FUNCTION ranges should never intersect or overlap. The current implementation *only* allows such use that the generated output contains overlapping ones, which is invalid (and also ignored by windows). Additionally, the current syntax is not suitable for implementing any correct behavior, as the chained function chunk should refer to the info or function it chains to, which is not done in the current syntax. A search on search engines and github show that this macro wasn't used outside of LLVM (which is understandable as it never worked) so I think this warrants the NFC tag.

The tests also exhibit the incorrect behavior, and pass since the offsets are regexed out so overlapping ranges don't fail.

Diff Detail

Unit TestsFailed

TimeTest
80 msx64 debian > LLVM.Bindings/Go::go.test
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/llvm-go go=/usr/bin/go test llvm.org/llvm/bindings/go/llvm

Event Timeline

namazso created this revision.Dec 16 2021, 10:30 PM
namazso requested review of this revision.Dec 16 2021, 10:30 PM
mstorsjo added subscribers: rnk, mstorsjo.

I think this is reasonable - I'm not familiar with the chained stuff, but the arguments sound sensible. Adding @rnk who I think might be more familiar with it (or might know who's the best authority on the matter).

(When uploading diffs, it's preferrable if the diffs are created with lots of extra context, like git diff -U999 or similar.)

rnk added a comment.Jan 6 2022, 2:00 PM

I think the intended usage model is supposed to be something like this:

.section .text, ...
.seh_proc foo
  push ...
  push ...
.seh_endprologue
  j...
  ret

.pushsection .text$cold, ...  # switch to new section (discontiguous)
.seh_startchained # implicitly references the open unwind info for foo
.seh_endprologue
  mov ...  # cold code moved out of line to separate section
  mov ...
.seh_endchained # end chained region

.popsection # return to main text section
.seh_endproc # main end label is in main section

The test doesn't do a good job representing proper usage, since it puts everything in the same section (.text) and the chained region has no code in it.

What do you think of the example? Is that a possible way that one could set up correct chained unwind info with the directives as they are? Unfortunately, I'm not familiar with these. As you can see, the compiler doesn't use them, they are only implemented at the assembler level.

What do you think of the example? Is that a possible way that one could set up correct chained unwind info with the directives as they are? Unfortunately, I'm not familiar with these. As you can see, the compiler doesn't use them, they are only implemented at the assembler level.

If you mean how it is right now, no, because that will just error:

<unknown>:0: error: expected relocatable expression
<unknown>:0: error: expected relocatable expression

syntax-wise it might be possible to make it work like this though, although I think a syntax like .seh_proc procname, chainedprocname might be better as it allows you to generate output similar to some that MSVC generates if you want to.