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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: tools/llvm-objcopy/CopyConfig.h:66
+  StringRef SectionName;
+  unsigned long long Value;
+  uint8_t Type;
----------------
jhenderson wrote:
> 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.
Okay, I now see what the problem is. It's to do with the call to `getAsUnsignedInteger`, later on right?

I think this issue could be solved by defining an unsigned long version of `getAsUnsignedInteger`. That way, it should work regardless of the underlying type of `uint64_t`. Implementing that would be my preference to working around this by using primitive types in llvm-objcopy rather than fixed-width types.


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

https://reviews.llvm.org/D58234