[PATCH] D55951: [LLD][COFF] Fix file/line retrieval when a undefined symbol is to be printed

[Reid Kleckner via Phabricator via llvm-commits]

rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.

lgtm



================
Comment at: COFF/PDB.cpp:1739
+  Optional<uint32_t> NameIndex;
+  Optional<uint32_t> LineNumber;
   for (LineColumnEntry &Entry : Lines) {
----------------
aganea wrote:
> aganea wrote:
> > rnk wrote:
> > > I think zero initializing these both is better than using Optional. The string table index 0 gives the empty string, right? It seems nicer to use that and avoid the extra conditional branches. I guess you'd still need the branch in the loop to say, if we don't have a filename so far but we have an entry, use the next entry. Although, the branchless way to write that is to pull the first filename string table index from the first line table entry if it exists.
> > Unfourtunately it's a 0-based index in the checksums table:
> > ```
> > *** FILECHKSUMS
> > 
> > FileId  St.Offset  Cb  Type  ChksumBytes
> >      0  00000001   10  MD5   D72EDEF8B8E50C364A330F9CB3CD904B
> >     18  00000065   10  MD5   D72EDEF8B8E50C364A330F9CB3CD904B
> >     30  000000A5   10  MD5   D72EDEF8B8E50C364A330F9CB3CD904B
> >     48  000000E8   10  MD5   D72EDEF8B8E50C364A330F9CB3CD904B
> > ```
> > The string table indeed starts with an empty string:
> > ```
> > *** STRINGTABLE
> > 
> > 00000000 
> > 00000001 C:\fastbuild-work\.fbuild.tmp\source\(edited).h
> > 00000065 C:\fastbuild-work\.fbuild.tmp\source\(edited).h
> > 000000a5 C:\fastbuild-work\.fbuild.tmp\source\(edited).h
> > 000000e8 C:\fastbuild-work\.fbuild.tmp\source\(edited).h
> > 0000014a C:\fastbuild-work\.fbuild.tmp\source\(edited).h
> > ```
> Essentially, if you zero-out `NameIndex` (which I did at first), you end up with the wrong error message (the first filename, instead of nothing)
Right, wrong index.


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55951/new/

https://reviews.llvm.org/D55951