[PATCH] D57006: [llvm-objcopy] [COFF] Update symbol indices in weak externals

[James Henderson via Phabricator via llvm-commits]

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

LGTM. I'm happy with you updating the last createStringError call if you want, or leaving as is.



================
Comment at: tools/llvm-objcopy/COFF/Reader.cpp:155-156
+      if (*Sym.WeakTargetSymbolId >= RawSymbolTable.size())
+        return make_error<StringError>("Weak external reference out of range",
+                                       object_error::parse_failed);
+      const Symbol *Target = RawSymbolTable[*Sym.WeakTargetSymbolId];
----------------
mstorsjo wrote:
> jhenderson wrote:
> > Use `createStringError` instead of `make_error<StringError>`
> Ok, I can change that.
> 
> I've used a lot of `make_error<StringError>` so far in the COFF backend, so there's a number of other occurrances of it that I could change in a separate patch if you prefer that as well? (And there's one instance of it in the ELF subdir.)
That would be good.


================
Comment at: tools/llvm-objcopy/COFF/Writer.cpp:83-85
+        return make_error<StringError>("Symbol " + Sym.Name +
+                                           " is missing its weak target",
+                                       object_error::invalid_symbol_index);
----------------
mstorsjo wrote:
> jhenderson wrote:
> > Ditto. In this case, you can then take advantage of the printf-style overload to avoid the string concatenation here.
> > 
> > Also putting some quotes around the symbol name might be good, especially if it's theoretically possible to have spaces in your symbol names in COFF.
> With printf-style formatting, I'd either need to make the StringRef a null terminated string, or use the `%.*s` formatting syntax with `Sym.Name.size(), Sym.Name.data()` or something similar, which kinda negates any convenience of printf-style formatting strings.
> 
> About quotes around the symbol names - oh, right, yes. It seems I've slipped in a few more of these before as well, I can push an NFC patch that fixes those, the next time I push something.
You could use StringRef::str() to avoid this.

Updating the other errors later would be good.


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

https://reviews.llvm.org/D57006