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

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo marked 4 inline comments as done.
mstorsjo added inline comments.


================
Comment at: test/tools/llvm-objcopy/COFF/weak-external.test:26-27
+  - Name:            .text
+    Characteristics: [  ]
+    Alignment:       4
+    SectionData:     C3C3
----------------
jhenderson wrote:
> If these two fields aren't required, it would be nice to get rid of them.
Apparently the test works even without them - will remove.


================
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];
----------------
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.)


================
Comment at: tools/llvm-objcopy/COFF/Writer.cpp:78
     }
+    if (Sym.WeakTargetSymbolId && Sym.Sym.NumberOfAuxSymbols == 1) {
+      coff_aux_weak_external *WE =
----------------
jhenderson wrote:
> This seems like a weird condition. Could you explain it please (e.g. with a comment).
I can add a comment, sure. The `NumberOfAuxSymbols` check needs to be at least greater than or equal to one for the code not to write out of bounds, but I've made the check stricter since only exactly equal to one makes sense.


================
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);
----------------
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.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57006