[PATCH] D58234: [llvm-objcopy] Add --add-symbol

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: tools/llvm-objcopy/CopyConfig.cpp:217-221
+  // The following flags are silently ignored and provided for GNU
+  // compatibility only:
+  //
+  // warning, debug, constructor, indirect, synthetic,
+  // unique-object, before=<symbol>.
----------------
I'd fold this bit of the comment in with the <flags> bit above, since they are closely linked. Also, I'd remove the word "silently" from it.


================
Comment at: tools/llvm-objcopy/CopyConfig.cpp:242
+  for (size_t I = 1; I < NumFlags; ++I)
+    if (Flags[I].equals_lower("global"))
+      SI.Bind = ELF::STB_GLOBAL;
----------------
Could you use a `StringSwitch` here, using `CaseLower` to get case insensitivity, perhaps with lambdas, to avoid the if/else list?


================
Comment at: tools/llvm-objcopy/CopyConfig.h:66
+  StringRef SectionName;
+  unsigned long long Value;
+  uint8_t Type;
----------------
evgeny777 wrote:
> rupprecht wrote:
> > uint64_t
> Like I said in D58173 this will not work, because `uint64_t` is not `unsigned long long` on 64 bit platforms
I don't understand what you mean by this? Symbol values are by definition in the ELF spec up to 64-bit in size, so `uint64_t` (which is 64-bits in size) is the correct type. `unsigned long long` is only guaranteed to be at least as big as `unsigned long` which in turn is only guaranteed to be as big as `unisgned int` etc.

Related note: The bitness of the platform does not define the size of types by itself. For example, on Windows, `unsigned long` is always 32 bits, both for 32-bit Windows and 64-bit Windows. On the other hand, `unsigned long long` is 64 bits on both.


================
Comment at: tools/llvm-objcopy/ELF/ELFObjcopy.cpp:573
+    Obj.SymbolTable->addSymbol(SI.SymbolName, SI.Bind, SI.Type, Sec, Value,
+                               SI.Visibility, SHN_ABS, 0);
+  }
----------------
evgeny777 wrote:
> rupprecht wrote:
> > GNU docs says: "If the section is given, the symbol will be associated with and relative to that section, otherwise it will be an ABS symbol."
> > 
> > I think SHN_ABS is wrong if the section name is given.
> The shndx parameter is ignored when `DefinedIn` is not nullptr. See `addSymbol`
This should probably be commented. It also feels rather fragile though.


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

https://reviews.llvm.org/D58234