[lld] Fixed CodeView GuidAdapter::format to handle GUID bytes in the right order.

[aorlov]

Summary

This fixes https://bugs.llvm.org/show_bug.cgi?id=41712 bug.

[aganea]

llvm/lib/DebugInfo/CodeView/Formatters.cpp

28

I know the patch you're proposing is more optimal in terms of runtime performance, but it adds an extra layer of complexity for a reader who was to figure out why memory is read in that way. Since GUIDs are not that used in binary files, performance is maybe less of a concern. We could maybe rewrite the code to be more self-documenting, something along those lines?

void GuidAdapter::format(raw_ostream &Stream, StringRef Style) {
  assert(Item.size() == 16 && "Expected 16-byte GUID");
  struct _GUID {
    support::ulittle32_t Data1;
    support::ulittle16_t Data2;
    support::ulittle16_t Data3;
    support::ubig64_t Data4;
  };
  const _GUID *G = reinterpret_cast<const _GUID *>(Item.data());
  Stream << "{"
         << format_hex_no_prefix(G->Data1, sizeof(G->Data1), /*Upper=*/true)
         << '-'
         << format_hex_no_prefix(G->Data2, sizeof(G->Data2), /*Upper=*/true)
         << '-'
         << format_hex_no_prefix(G->Data3, sizeof(G->Data3), /*Upper=*/true)
         << '-'
         << format_hex_no_prefix(G->Data4, sizeof(G->Data4), /*Upper=*/true)
         << "}";
}

[rnk]

llvm/lib/DebugInfo/CodeView/Formatters.cpp

28

+1

[aganea]

Thanks for integrating the comments!

llvm/lib/ObjectYAML/CodeViewYAMLTypes.cpp

163 ↗

(On Diff #336116)

In order to avoid any magical offsets into the string, can you possibly keep a loop, but add a counter and successively write into Data1, Data2, Data3, etc.? Something like:

uint8_t *OutBuffer = S.Guid;
unsigned Index = 0;
uint64_t D41{}, D42{};
bool GotHex;
for (auto Iter = Scalar.begin(); Iter != Scalar.end(); ++Iter) {
  auto Next = std::find_if(Iter, Scalar.end(), std::isxdigit);
  if (Next == Scalar.end())
    break;
  StringRef Group(Iter, Next - Iter);
  if (Index == 0)
    GotHex = to_integer(Group, G.Data1, 16);
  else if (Index == 1)
    GotHex &= to_integer(Group, G.Data2, 16);
  else if (Index == 2)
    GotHex &= to_integer(Group, G.Data3, 16);
  else if (Index == 3)
    GotHex &= to_integer(Group, D41, 16);
  else if (Index == 4)
    GotHex &= to_integer(Group, D42, 16);
  ++Index;
}
G.Data4 = (D41 << 48) | D42;

[aorlov]

llvm/lib/ObjectYAML/CodeViewYAMLTypes.cpp

163 ↗

(On Diff #336116)

No magical offsets, no loops. But I kept the maximal validation.

[aganea]

LGTM. Thank you!

llvm/lib/DebugInfo/CodeView/Formatters.cpp

 GuidAdapter::GuidAdapter(StringRef Guid)
     : FormatAdapter(makeArrayRef(Guid.bytes_begin(), Guid.bytes_end())) {}
 
 GuidAdapter::GuidAdapter(ArrayRef<uint8_t> Guid)
     : FormatAdapter(std::move(Guid)) {}
 
 void GuidAdapter::format(raw_ostream &Stream, StringRef Style) {
   static const char *Lookup = "0123456789ABCDEF";
+  static const uint8_t Order[16] = {3, 2, 1,  0,  5,  4,  7,  6,
+                                    8, 9, 10, 11, 12, 13, 14, 15};
 
   assert(Item.size() == 16 && "Expected 16-byte GUID");
   Stream << "{";
   for (int i = 0; i < 16;) {
-    uint8_t Byte = Item[i];
+    uint8_t Byte = Item[Order[i]];
     uint8_t HighNibble = (Byte >> 4) & 0xF;
     uint8_t LowNibble = Byte & 0xF;
     Stream << Lookup[HighNibble] << Lookup[LowNibble];
     ++i;
     if (i >= 4 && i <= 10 && i % 2 == 0)
       Stream << "-";
   }
   Stream << "}";
 }
 
 raw_ostream &llvm::codeview::operator<<(raw_ostream &OS, const GUID &Guid) {
   codeview::detail::GuidAdapter A(Guid.Guid);
   A.format(OS, "");
   return OS;
 }

llvm/test/tools/llvm-pdbutil/explain-pdb-stream.test

 CHECK-NEXT:     The current value is 1.
 
 CHECK:      Block:Offset = 11:000C.
 CHECK-NEXT: Address is in block 17 (allocated).
 CHECK-NEXT:   Address is at offset 12/202 of Stream 1 (PDB Stream).
 CHECK-NEXT:   Within the PDB stream:
 CHECK-NEXT:     address is at offset 12/28 of the PDB Stream Header.
 CHECK-NEXT:     which contains the guid of the PDB.
-CHECK-NEXT:     The current value is {826BE46E-02ED-7043-9C27-20CCC07E92A7}.
+CHECK-NEXT:     The current value is {6EE46B82-ED02-4370-9C27-20CCC07E92A7}.
 
 CHECK:      Block:Offset = 11:001C.
 CHECK-NEXT: Address is in block 17 (allocated).
 CHECK-NEXT:   Address is at offset 28/202 of Stream 1 (PDB Stream).
 CHECK-NEXT:   Within the PDB stream:
 CHECK-NEXT:     address is at offset 0/166 of the Named Stream Map.

llvm/test/tools/llvm-pdbutil/stripped.test

 ; RUN: llvm-pdbutil dump -all %p/Inputs/Stripped.pdb > %t
 ; RUN: FileCheck -input-file=%t %s
 
 ; CHECK: Summary
 ; CHECK-NEXT: ============================================================
 ; CHECK-NEXT:  Block Size: 4096
 ; CHECK-NEXT:  Number of blocks: 17
 ; CHECK-NEXT:  Number of streams: 12
 ; CHECK-NEXT:  Signature: 1541179274
 ; CHECK-NEXT:  Age: 2
-; CHECK-NEXT:  GUID: {FF4F9B62-D99A-4647-97A7-22C702B1E053}
+; CHECK-NEXT:  GUID: {629B4FFF-9AD9-4746-97A7-22C702B1E053}
 ; CHECK-NEXT:  Features: 0x1
 ; CHECK-NEXT:  Has Debug Info: true
 ; CHECK-NEXT:  Has Types: true
 ; CHECK-NEXT:  Has IDs: true
 ; CHECK-NEXT:  Has Globals: true
 ; CHECK-NEXT:  Has Publics: true
 ; CHECK-NEXT:  Is incrementally linked: false
 ; CHECK-NEXT:  Has conflicting types: false