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

[Reid Kleckner via Phabricator via llvm-commits]

rnk marked 3 inline comments as done.
rnk added inline comments.


================
Comment at: lld/COFF/Driver.cpp:1537
+  if (Config->Machine == I386 &&
+      Args.hasFlag(OPT_safeseh, OPT_safeseh_no, true))
+    Config->SafeSEH = true;
----------------
mstorsjo wrote:
> mstorsjo wrote:
> > Should the default be off in the mingw case, if linking objects from gcc?
> 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.
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.


================
Comment at: lld/COFF/Driver.cpp:1765
-  // Handle /safeseh.
-  if (Args.hasFlag(OPT_safeseh, OPT_safeseh_no, false)) {
-    for (ObjFile *File : ObjFile::Instances)
----------------
thakis wrote:
> Huh, why was the default here false?
Unclear, but it probably had to be since it wouldn't have worked at all for mingw.


================
Comment at: lld/COFF/Writer.cpp:1431
+      // safeseh flag.
+      SetNoSEHCharacteristic = false;
+      continue;
----------------
thakis wrote:
> mstorsjo wrote:
> > 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.
> I agree, I would've expected an early return instead. (e.g. the `if (ErrorCount) return` after the loop that the lhs has)
I guess I was anticipating having to downgrade this to a warning. I'll remove it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63570/new/

https://reviews.llvm.org/D63570