[PATCH] D42592: [COFF] Add minimal support for /guard:cf

[Reid Kleckner via Phabricator via llvm-commits]

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


================
Comment at: lld/COFF/Chunks.h:326
+// later.
+using SymbolRVASet = llvm::DenseSet<std::pair<Chunk *, uint32_t>>;
+
----------------
amccarth wrote:
> 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.
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?


================
Comment at: lld/COFF/InputFiles.h:191
+  // not linked into the final binary when /guard:cf is set.
+  std::vector<SectionChunk *> GuardFidChunks;
+
----------------
pcc wrote:
> pcc wrote:
> > Can there really be more than one `.gfids$y` section? I don't see why that would be needed.
> 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;
> ```
Yep.


================
Comment at: lld/COFF/Writer.cpp:874
 
+void Writer::createGFIDTable(OutputSection *RData) {
+  SymbolRVASet AddressTakenSyms;
----------------
ruiu wrote:
> 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.
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.


================
Comment at: lld/test/COFF/gfids-gc.s:60
+# CHECK-GC:   0x14000{{.*}}
+# CHECK-GC: ]
+
----------------
amccarth wrote:
> 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.
There's a `GuardCFFunctionCount: 2` line above, but -NEXT here is probably also appropriate.


================
Comment at: lld/test/COFF/gfids-icf.s:6
+# There will be two address taken functions: main and icf1. icf2 will be
+# eliminated.
+# CHECK: ImageBase: 0x140000000
----------------
amccarth wrote:
> 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?)
This should be clearer!


https://reviews.llvm.org/D42592