This is an archive of the discontinued LLVM Phabricator instance.

[COFF] Implement /safeseh:no and check @feat.00 flags by default
ClosedPublic

Authored by rnk on Jun 19 2019, 1:20 PM.

Details

Summary

Fixes PR41828. Before this, LLD always emitted SafeSEH chunks and
defined __safe_se_handler_table & size. Now, /safeseh:no leaves those
undefined.

Additionally, we were checking for the safeseh @feat.00 flag in two
places: once to emit errors, and once during safeseh table construction.
The error was set up to be off by default, but safeseh is supposed to be
on by default. I combined the two checks, so now LLD emits an error if
an input object lacks @feat.00 and safeseh is enabled. This caused the
majority of 32-bit LLD tests to fail, since many test input object files
lack @feat.00 symbols. I explicitly added -safeseh:no to those tests to
preserve behavior.

Finally, LLD no longer sets IMAGE_DLL_CHARACTERISTICS_NO_SEH if any
input file wasn't compiled for safeseh.

Diff Detail

Repository
rL LLVM

Event Timeline

rnk created this revision.Jun 19 2019, 1:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 19 2019, 1:20 PM
mstorsjo added inline comments.Jun 25 2019, 8:48 PM
lld/COFF/Driver.cpp
1537 ↗(On Diff #205661)

Should the default be off in the mingw case, if linking objects from gcc?

lld/COFF/Writer.cpp
1431 ↗(On Diff #205661)

Changing this flag for writing different output, right after flagging an error (which will, eventually, abort the writing?) feels a bit weird, but I guess it's good for completeness.

thakis accepted this revision.Jun 26 2019, 4:20 AM

Thanks for doing this1

lld/COFF/Driver.cpp
1765 ↗(On Diff #205661)

Huh, why was the default here false?

lld/COFF/Writer.cpp
1431 ↗(On Diff #205661)

I agree, I would've expected an early return instead. (e.g. the if (ErrorCount) return after the loop that the lhs has)

This revision is now accepted and ready to land.Jun 26 2019, 4:20 AM
ruiu accepted this revision.Jun 26 2019, 4:27 AM

LGTM

Sorry, I thought that I reviewed this alreayd.

mstorsjo requested changes to this revision.Jul 1 2019, 2:22 PM
mstorsjo added inline comments.
lld/COFF/Driver.cpp
1537 ↗(On Diff #205661)

I tested this patch now, and indeed, either the default needs to be different if Config->MinGW is true, or the MinGW driver needs to pass -safeseh:no by default.

This revision now requires changes to proceed.Jul 1 2019, 2:22 PM
rnk marked 3 inline comments as done.Jul 15 2019, 4:05 PM
rnk added inline comments.
lld/COFF/Driver.cpp
1537 ↗(On Diff #205661)

I figured as much. :) Glad I waited. I think today for mingw we silently don't emit a safeseh table, because we stop as soon as we find an object file without the @feat.00 bit set. So, I think if we change this default to !Config->Mingw, that will be the smallest change in behavior.

I think it might be useful to expose a mode for mingw that assumes that object files that lack the @feat.00 bit have no SEH, but that would be a new feature.

1765 ↗(On Diff #205661)

Unclear, but it probably had to be since it wouldn't have worked at all for mingw.

lld/COFF/Writer.cpp
1431 ↗(On Diff #205661)

I guess I was anticipating having to downgrade this to a warning. I'll remove it.

rnk updated this revision to Diff 209990.Jul 15 2019, 4:14 PM
  • change default
  • simplify
mstorsjo accepted this revision.Jul 16 2019, 4:20 AM

LGTM, although you still have to rebase it past the change to camelBack casing of variables :-)

lld/test/COFF/dllexport-mingw.s
5 ↗(On Diff #209990)

Aren't many of the test changes redundant, in the places where we have -lldmingw? Although it might be good for consistency...

This revision is now accepted and ready to land.Jul 16 2019, 4:20 AM
rnk updated this revision to Diff 210138.Jul 16 2019, 11:14 AM
  • rebase
This revision was automatically updated to reflect the committed changes.