This is an archive of the discontinued LLVM Phabricator instance.

[COFF] Add minimal support for /guard:cf
ClosedPublic

Authored by rnk on Jan 26 2018, 11:44 AM.

Details

Summary

This patch adds some initial support for Windows control flow guard. At
the end of the day, the linker needs to synthesize a table of RVAs very
similar to the structured exception handler table (/safeseh).

Both /safeseh and /guard:cf take sections of symbol table indices
(.sxdata and .gfids$y) and turn them into RVA tables referenced by the
load config struct in the CRT through special symbols.

Event Timeline

rnk created this revision.Jan 26 2018, 11:44 AM
pcc added a reviewer: pcc.Jan 26 2018, 11:58 AM
pcc added a subscriber: pcc.
pcc added inline comments.
lld/COFF/InputFiles.h
180

Can there really be more than one .gfids$y section? I don't see why that would be needed.

pcc added inline comments.Jan 26 2018, 1:20 PM
lld/COFF/InputFiles.h
180

Hmm, never mind, I see that we can get multiple sections like this:

void a(), b();
__declspec(selectany) void* f = a;
__declspec(selectany) void* g = b;
ruiu added inline comments.Jan 26 2018, 1:23 PM
lld/COFF/Chunks.cpp
459–460

Please avoid auto even if writing a real type is a bit too lengthy. If you want to use auto, please do something like this so that a real type is explicit in a local context.

for (const auto &P : Syms) {
  Defined *Sym = P.first;
  ....
}
lld/COFF/DriverUtils.cpp
132–136

You can use StringRef::equals_lower.

lld/COFF/InputFiles.h
104–109

Since these enums are used only by ObjFile, I'd move this into that class (and make them a non-enum class.)

135–142

Actually, I'd even use 1 and 0x800 instead of defining them as constants, as they are used only once and already factored out well.

lld/COFF/Writer.cpp
896

Can you write a function comment to give a big picture? Once it becomes clear as to what this function should create, the details becomes obvious.

Look nice! A couple nits.

lld/COFF/Chunks.h
326

Using a std::pair rather than a struct with named fields makes the usage of these (in the .cpp file) a bit harder to follow.

lld/test/COFF/gfids-gc.s
61

This just checks that there are at least two entries in the GuardFidTable, but not whether there're exactly two. (You'd need CHECK-GC-NEXT, right?) Was that intentional?

I supposed checking that they're are the _right_ two is a bit of overkill.

lld/test/COFF/gfids-icf.s
7

The other test was a little easier to understand because it showed the approximate source code. If that's infeasible here, could you extend the the comment to say something like, "Identical comdat folding will eliminate icf2 because it's the same as icf1."

(I'm not even sure that's exactly true. Is there a guarantee about _which_ copy will be eliminated or just that one of them will?)

rnk marked 9 inline comments as done.Feb 5 2018, 4:41 PM
rnk added inline comments.
lld/COFF/Chunks.h
326

I implemented this, but it meant I had to implement DenseMapInfo type traits, which uglified things a fair amount. I think this is OK, but what do you think?

lld/COFF/InputFiles.h
180

Yep.

lld/COFF/Writer.cpp
896

I did this.

One other consideration here is that this is Yet Another loop over all input object files. If this work shows up in profiling, we should consider deleting this set and making it a bit on Symbol just like WrittenToSymtab.

lld/test/COFF/gfids-gc.s
61

There's a GuardCFFunctionCount: 2 line above, but -NEXT here is probably also appropriate.

lld/test/COFF/gfids-icf.s
7

This should be clearer!

rnk updated this revision to Diff 132911.Feb 5 2018, 4:41 PM
rnk marked 4 inline comments as done.
  • address feedback
amccarth accepted this revision.Feb 5 2018, 4:56 PM

LGTM.

This revision is now accepted and ready to land.Feb 5 2018, 4:56 PM
ruiu added a comment.Feb 5 2018, 5:07 PM

Generally looking pretty good. Minor comments.

lld/COFF/Writer.cpp
868

Can you write a function comment for this function so that readers can get a big picture?

896

I'm not too worried about a loop that loops over input files because the number of input files isn't that big. Of course your mileage may vary -- if each iteration of the loop is heavy, it would appear in the profiler. Let's see how it works.

927

Ditto

941

Likewise, toString(C) is probably better.

943

toString(File) is better because it generates a user-friendly string even for object files in archive files.

952

ulittle32_t is a small object so I wouldn't use it through a reference. I'd just use uint32_t here.

rnk marked 4 inline comments as done.Feb 5 2018, 5:36 PM
rnk added inline comments.
lld/COFF/Writer.cpp
941

As discussed, this overload doesn't exist

rnk updated this revision to Diff 132924.Feb 5 2018, 5:44 PM
  • address feedback
ruiu accepted this revision.Feb 5 2018, 5:51 PM

LGTM

lld/COFF/Writer.cpp
952

Is this done?

rnk added a comment.Feb 5 2018, 5:59 PM

Thanks!

lld/COFF/Writer.cpp
952

Yes, I think I forgot to quit the editor prompt when I uploaded the next patch.

This revision was automatically updated to reflect the committed changes.