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

[Adrian McCarthy via Phabricator via llvm-commits]

amccarth added a comment.

Look nice!  A couple nits.



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


================
Comment at: lld/test/COFF/gfids-gc.s:60
+# CHECK-GC:   0x14000{{.*}}
+# CHECK-GC: ]
+
----------------
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.


================
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
----------------
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?)


https://reviews.llvm.org/D42592