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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

Generally looking pretty good. Minor comments.



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


================
Comment at: lld/COFF/Writer.cpp:868
+
+static void markSymbolsWithRelocations(ObjFile *File,
+                                       SymbolRVASet &UsedSymbols) {
----------------
Can you write a function comment for this function so that readers can get a big picture?


================
Comment at: lld/COFF/Writer.cpp:924
+
+void Writer::markSymbolsForRVATable(ObjFile *File,
+                                    ArrayRef<SectionChunk *> SymIdxChunks,
----------------
Ditto


================
Comment at: lld/COFF/Writer.cpp:938
+    if (Data.size() % 4 != 0) {
+      warn("ignoring " + C->getSectionName() +
+           " symbol table index section in object " +
----------------
Likewise, `toString(C)` is probably better.


================
Comment at: lld/COFF/Writer.cpp:940
+           " symbol table index section in object " +
+           sys::path::filename(File->getName()));
+      continue;
----------------
`toString(File)` is better because it generates a user-friendly string even for object files in archive files.


================
Comment at: lld/COFF/Writer.cpp:949
+    ArrayRef<Symbol *> ObjSymbols = File->getSymbols();
+    for (const ulittle32_t &SymIndex : SymIndices) {
+      if (SymIndex >= ObjSymbols.size()) {
----------------
ulittle32_t is a small object so I wouldn't use it through a reference. I'd just use `uint32_t` here.


https://reviews.llvm.org/D42592