[PATCH] D149268: [LLD][COFF] Fix PDB relocation on big-endian hosts

Thu Apr 27 06:53:59 PDT 2023

aganea added a reviewer: mstorsjo.
aganea added inline comments.

================
Comment at: lld/COFF/PDB.cpp:958
     auto unrelocatedRvaStart = subsecData.take_front(sizeof(uint32_t));
     uint8_t relocatedRvaStart[sizeof(uint32_t)];
     debugChunk->writeAndRelocateSubsection(debugChunk->getContents(),
----------------
uweigand wrote:
> aganea wrote:
> > Can't we just write `support::ulittle32_t rvaStart;` here and static cast to a uint8_t* it below in the function call?
> I guess that would be safe (in particular w.r.t. aliasing rules) if `uint8_t` is implemented as an `unsigned char`.  This is probably always true in practice, but may not be absolutely guaranteed by the C++ standard.  Maybe this is why the original code used a memcpy here?
> 
> Let me know if you want me to make that change.   (The generated code should be identical with all modern compilers anyway.)
> 
I feel this could help future maintainers of this code. The expectations should be clear at the caller site, and it is not quite clear now why storage for uint32_t has be allocated through a uint8_t, just to memcpy it back into a `ulittle32_t` after the call. This all feels quite strange. If we want to avoid the casting here maybe `writeAndRelocateSubsection` should take a `void*` and let it deal with casting internally?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149268