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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


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


================
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];
----------------
Use `createStringError` instead of `make_error<StringError>`


================
Comment at: tools/llvm-objcopy/COFF/Reader.cpp:159-160
+      if (Target == nullptr)
+        return make_error<StringError>("Invalid SymbolTableIndex",
+                                       object_error::parse_failed);
+      Sym.WeakTargetSymbolId = Target->UniqueId;
----------------
Ditto.


================
Comment at: tools/llvm-objcopy/COFF/Writer.cpp:78
     }
+    if (Sym.WeakTargetSymbolId && Sym.Sym.NumberOfAuxSymbols == 1) {
+      coff_aux_weak_external *WE =
----------------
This seems like a weird condition. Could you explain it please (e.g. with a comment).


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


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57006