Page MenuHomePhabricator

[CodeGen] [WinException] Only produce handler data at the end of the function if needed
ClosedPublic

Authored by mstorsjo on Sep 10 2020, 5:03 AM.

Details

Summary

If we are going to write handler data (that is written as variable length data following after the unwind info in .xdata), we need to emit the handler data immediately, but for cases where no such info is going to be written, skip emitting it right away. (Unwind info for all remaining functions that hasn't gotten it emitted directly is emitted at the end.)

This does slightly change the ordering of sections (triggering a bunch of updates to DebugInfo/COFF tests), but the change should be benign.

This also matches GCC's assembly output, which doesn't output .seh_handlerdata unless it actually is needed.

For ARM64, the unwind info can be packed into the runtime function entry itself (leaving no data in the .xdata section at all), but that can only be done if there's no follow-on data in the .xdata section. If emission of the unwind info is triggered via EmitWinEHHandlerData (or the .seh_handlerdata directive), which implicitly switches to the .xdata section, there's a chance of the caller wanting to pass further data there, so the packed format can't be used in that case.

This turned out to be the reason why D87371 had no effect at all, as it forced .seh_handlerdata at the end of all functions. With this patch in place, D87371 has got a small effect (but very few functions in practice end up matching).

Diff Detail

Event Timeline

mstorsjo created this revision.Sep 10 2020, 5:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 10 2020, 5:03 AM
rnk added a comment.Sep 10 2020, 2:19 PM

Makes sense.

Unwind info for all remaining functions that hasn't gotten it emitted directly is emitted at the end.

Can we make .seh_endproc responsible for emitting the unwind info into .xdata if necessary? Would that preserve the section ordering? There is something nice about having all the .text/.pdata/.xdata sections grouped together in the object file, even though the order doesn't matter.

llvm/test/CodeGen/X86/mingw-comdats.ll
81

Shouldn't this remain? This change seems unrelated.

In D87448#2266746, @rnk wrote:

Makes sense.

Unwind info for all remaining functions that hasn't gotten it emitted directly is emitted at the end.

Can we make .seh_endproc responsible for emitting the unwind info into .xdata if necessary?

I guess it could be done, but... Right now, EmitWinCFIEndProc is only implemented in the MCStreamer base class, while the class responsible for actually writing the .xdata contents, Win64EH::UnwindEmitter or Win64EH::ARM64UnwindEmitter) is in the subclasses X86WinCOFFStreamer and AArch64WinCOFFStreamer, so it'd require overriding EmitWinCFIEndProc in these subclasses and calling the respective UnwindEmitter methods for writing the xdata right there. Not hard, but a bit extra boilerplate needed in any target specific subclass doing this, if they're to be consistent wrt this.

Would that preserve the section ordering? There is something nice about having all the .text/.pdata/.xdata sections grouped together in the object file, even though the order doesn't matter.

I guess it would - but even right now, they're not grouped together right now. For an object file with two comdat functions, func1 and func2, GCC/binutils do generate them in the order .text$func1, .xdata$func1, .pdata$func1, .text$func2, .xdata$func2, .pdata$func2 - i.e .text/.xdata/.pdata ordered together.

Currently, LLVM generates .text$func1, .xdata$func1, .text$func2, .xdata$func2, .pdata$func1, .pdata$func2 - i.e. .text/.xdata pairwise together, but all .pdata emitted at the end. With this patch in place, it generates .text$func1, .text$func2, .xdata$func1, .xdata$func2, .pdata$func1, .pdata$func2, which admittedly is the least nice ordering.

In that sense, the current ordering isn't much to strive for either, so if we want to make an effort towards keeping that, we probably also should emit the .pdata right next after the corresponding .xdata. I don't think that's much extra work either, but still a little bigger refactoring than just "stop emitting unnecessary .seh_handlerdata and keep .xdata where it was".

llvm/test/CodeGen/X86/mingw-comdats.ll
81

It was a mistake on my side; it does remain, but the sections have changed order, and I thought it wasn't important to the testcase (like unrelated lines in the middle of a CHECK-NEXT sequence), but I see it's part of what it actually tries to check. I've locally updated the patch to move it up between text and xdata.

I think it's called this out in the comments on the aarch64 implementation, but ultimately, I think we want specialized MCFragments for pdata/xdata. The current AArch64 implementation currently has a couple FIXMEs which can't be fixed in the current architecture (in particular, it doesn't interact with relaxation correctly).

@rnk - Can you follow up on the discussion above?

rnk accepted this revision.Sep 21 2020, 12:00 PM

@rnk - Can you follow up on the discussion above?

I agree, it sounds like the current order isn't worth preserving then. Let's do this.

This revision is now accepted and ready to land.Sep 21 2020, 12:00 PM