This is an archive of the discontinued LLVM Phabricator instance.

Make x86 __ehhandler comdat if parent function is
ClosedPublic

Authored by kastiglione on Oct 15 2017, 4:19 PM.

Details

Summary

This change comes from using lld for i686-windows-msvc. Before this change, lld
emits an error of:

error: relocation against symbol in discarded section: .xdata

It's possible that this could be addressed in lld, but I think this change is
reasonable on its own.

At a high level, this is being generated:

A (.text comdat) -> B (.text) -> C (.xdata comdat)

Where A is a C++ inline function, which references B, an exception handler
thunk, which references C, the exception handling info.

With this structure, lld will error when applying relocations to B if the C it
references has been discarded (some other C has been selected).

This change checks if A is comdat, and if so places the exception registration
thunk (B) in the comdata group of A (and B).

It appears that MSVC makes the __ehhandler function comdat.

Is it possible that duplicate thunks are being emitted into the final binary
with other linkers, or are they stripping the unused thunks?

Event Timeline

kastiglione created this revision.Oct 15 2017, 4:19 PM

Here's a reduced case: http://godbolt.org/g/rDEJcV

I'm going to add a test case. What's the best way? I'm thinking that I would take the reduced C++ code and use the generated IR to create assembly and then FileCheck against that.

compnerd edited edge metadata.Oct 15 2017, 4:26 PM

You should be able to just do an IR level check for this. You can ensure that both get placed into a COMDAT section.

BTW, I think that this is sufficient to show the behavior in MSVC:

struct S { ~S(); };
void g();

__declspec(dllexport) void f() {
  S s;
  g();
}
kastiglione added a comment.EditedOct 15 2017, 8:47 PM

do an IR level check for this

running clang -target i686-windows-msvc -S -emit-llvm -o - input.cpp generates IR for the function, but not for the __ehhandler thunk. I think the __ehhandler is encapsulated by the cleanuppad, and if so are there flags to get the IR through the passes that generate the thunk?

> BTW, I think that this is sufficient to show the behavior in MSVC:

In the code you provided, f and its xdata are not comdat. I believe the problem only arises when the xdata is comdat and then dropped by the linker, leaving the __ehhandler with relocations to a discarded section. The code I tested with uses an inline member function to generate a comdat function, and a free function to call and materialize the member function. This also mirrors the real code that I first saw this happen with.

smeenai edited edge metadata.Oct 15 2017, 9:39 PM

You can use /Gy (or -ffunction-sections if you're not using the cl-style driver) to force each function into its own COMDAT section.

rnk edited edge metadata.Oct 16 2017, 9:59 AM

Thanks! Needs a test case. It should be possible to test this directly with opt.

lib/Target/X86/X86WinEHState.cpp
404–405

It's probably more correct to take the comdat of ParentFunc directly, like this:

if (auto *C = ParentFunc->getComdat())
  Trampoline->setComdat(C);

This is unlikely to matter in practice. The only way I can think of to get into a situation where it matters is perhaps:

int g();
static int f() { // will make an __ehhandler and get inlined into the initializer for x
  try { return g(); } catch (...) { return 0; }
}
int __declspec(selectany) x = f(); // will create a dynamic initializer function comdat with 'x'

Reuse parent comdat; Add test

kastiglione marked an inline comment as done.Oct 17 2017, 11:17 AM
compnerd accepted this revision.Oct 17 2017, 1:44 PM
This revision is now accepted and ready to land.Oct 17 2017, 1:44 PM
rnk accepted this revision.Oct 18 2017, 9:36 AM

lgtm

This revision was automatically updated to reflect the committed changes.