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

[uweigand]

Summary

When running the LLD test suite on a big-endian host, the COFF/pdb-framedata.yaml test case currently fails.

As it turns out, this is because code in DebugSHandler::finish intended to relocate RvaStart entries of FDO records does not work correctly when compiled for a big-endian host.

Fixed by always reading file data in little-endian mode.

[aganea]

lld/COFF/PDB.cpp

958

Can't we just write support::ulittle32_t rvaStart; here and static cast to a uint8_t* it below in the function call?

[uweigand]

lld/COFF/PDB.cpp

958

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

[aganea]

lld/COFF/PDB.cpp

958

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?

[uweigand]

lld/COFF/PDB.cpp

958

I mean, that memcpy is a typical pattern to work around type-based aliasing violations potentially triggering undefined behavior, so my assumption is this is why the memcpy was used here. (To be clear, the problem is not the cast as such, it's the aliasing enabled by it, and therefore it doesn't matter w.r.t. undefined behavior whether or not the cast is done in the caller or the callee.)

What would make it safe without a memcpy would be to change the type of the buf argument to a char * (or unsigned char *) in writeAndRelocateSubsection and its subroutines and access it as such consistently. [ Or, as I said, simply make and accept the assumption that uint8_t is implemented as unsigned char on all supported platforms. ]

[aganea]

lld/COFF/PDB.cpp

958

Indeed I forgot about all this type punning. Since all the other usages of writeAndRelocateSubsection don't need punning let's leave it like this. Should we add a short comment to that effect above memcpy in case it isn't obvious for everyone?

[uweigand]

lld/COFF/PDB.cpp

958

Fair enough; added a comment now.

[aganea]

LGTM, thanks for the update!

lld/COFF/PDB.cpp

void DebugSHandler::finish() {
   ExitOnError exitOnErr;
 
   // Handle FPO data. Each subsection begins with a single image base
   // relocation, which is then added to the RvaStart of each frame data record
   // when it is added to the PDB. The string table indices for the FPO program
   // must also be rewritten to use the PDB string table.
   for (const UnrelocatedFpoData &subsec : frameDataSubsecs) {
     // Relocate the first four bytes of the subection and reinterpret them as a
-    // 32 bit integer.
+    // 32 bit little-endian integer.
     SectionChunk *debugChunk = subsec.debugChunk;
     ArrayRef<uint8_t> subsecData = subsec.subsecData;
     uint32_t relocIndex = subsec.relocIndex;
     auto unrelocatedRvaStart = subsecData.take_front(sizeof(uint32_t));
     uint8_t relocatedRvaStart[sizeof(uint32_t)];
     debugChunk->writeAndRelocateSubsection(debugChunk->getContents(),
                                            unrelocatedRvaStart, relocIndex,
                                            &relocatedRvaStart[0]);
-    uint32_t rvaStart;
-    memcpy(&rvaStart, &relocatedRvaStart[0], sizeof(uint32_t));
+    // Use of memcpy here avoids violating type-based aliasing rules.
+    support::ulittle32_t rvaStart;
+    memcpy(&rvaStart, &relocatedRvaStart[0], sizeof(support::ulittle32_t));
 
     // Copy each frame data record, add in rvaStart, translate string table
     // indices, and add the record to the PDB.
     DebugFrameDataSubsectionRef fds;
     BinaryStreamReader reader(subsecData, support::little);
     exitOnErr(fds.initialize(reader));
     for (codeview::FrameData fd : fds) {
       fd.RvaStart += rvaStart;