[PATCH] D45467: COFF: Friendlier undefined symbol errors.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: lld/COFF/Chunks.h:195-197
+  ArrayRef<coff_relocation> relocs() const {
+    return {Relocs.begin(), Relocs.end()};
+  }
----------------
zturner wrote:
> ruiu wrote:
> > If we can always do this, should we store Relocs as an ArrayRef instead of iterator_range in the first place?
> Alternatively, why not just return an `iterator_range`?
Switching to ArrayRef made the code a little simpler and was done in D45714.


================
Comment at: lld/COFF/PDB.cpp:1210
+
+    for (const DebugSubsectionRecord &SS : Subsections) {
+      switch (SS.kind()) {
----------------
zturner wrote:
> Don't we already iterate the subsection list for other reasons in this file?  Would it be worth piggybacking off of that to save the relevant info from the various subsections into each `SectionChunk` so that we can just do something like `DbgC->Lines`
I don't think that would work. We read CV subsections elsewhere to create a PDB, but that needs to happen after we issue undefined symbol errors because it requires section contents to be relocated, which requires all sections to be laid out and all symbols to be defined. But if we're issuing an undefined symbol error then obviously not all symbols will be defined.


================
Comment at: lld/COFF/PDB.h:33-34
+
+std::pair<llvm::StringRef, uint32_t> getFileLine(SectionChunk *C,
+                                                 uint32_t Addr);
 }
----------------
zturner wrote:
> Can you take this argument by `const&` instead of by non-const pointer?
Const seems fine but we usually pass pointers around in this part of the code for things like chunks.


https://reviews.llvm.org/D45467