[Debug-Info][CodeView] Fix GUID string generation for MSVC generated objects.

[CarlosAlbertoEnciso]

Summary

Fix invalid GUID string generation described by https://bugs.llvm.org/show_bug.cgi?id=50459

The valid format for a GUID is {XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX}
where X is a hex digit (0,1,2,3,4,5,6,7,8,9,A,B,C,D,E,F).

The length of the individual components must be: 8, 4, 4, 4, 12.

[amccarth]

This sounds like an appropriate place to apply Postel's Law: Be conservative in what you do, be liberal in what you accept from others. I'm be happy to always generate uppercase hex digits when formatting a GUID to registry format, but maybe we should also consider accepting upper or lowercase when reading a formatted GUID.

Some clarifications:

• GUIDs are numbers not strings. Microsoft claims they are essentially UUIDs (per RFC 4122).

• The cited document is actually specific to the string representation of GUIDs as required by Windows Installer. In fact, the braces indicate that it's the registry format. Windows installer--and other tools that use formatted GUIDs as registry keys--do string comparisons rather than converting the string representation to a numeric GUID that can be compared directly.

• The bit about using uppercase for the hex digits in the string representation does not apply everywhere, not even in the Microsoft world. Microsoft's GUIDGEN tool formats GUIDs in various ways with respect to the case of the hex digits. And in RFC 4122 says string representations of UUIDs should use lowercase.

[CarlosAlbertoEnciso]

In D103089#2779878, @amccarth wrote:

This sounds like an appropriate place to apply Postel's Law: Be conservative in what you do, be liberal in what you accept from others. I'm be happy to always generate uppercase hex digits when formatting a GUID to registry format, but maybe we should also consider accepting upper or lowercase when reading a formatted GUID.

Some clarifications:

• GUIDs are numbers not strings. Microsoft claims they are essentially UUIDs (per RFC 4122).

• The cited document is actually specific to the string representation of GUIDs as required by Windows Installer. In fact, the braces indicate that it's the registry format. Windows installer--and other tools that use formatted GUIDs as registry keys--do string comparisons rather than converting the string representation to a numeric GUID that can be compared directly.

• The bit about using uppercase for the hex digits in the string representation does not apply everywhere, not even in the Microsoft world. Microsoft's GUIDGEN tool formats GUIDs in various ways with respect to the case of the hex digits. And in RFC 4122 says string representations of UUIDs should use lowercase.

@amccarth Thanks for your feedback.

I have to upload a correct patch to include the binary files (Using arcanist), as the tests are failing due to invalid files.

[CarlosAlbertoEnciso]

Correct patch to include binary files used for the test.

[CarlosAlbertoEnciso]

In D103089#2779878, @amccarth wrote:

I'm be happy to always generate uppercase hex digits when formatting a GUID to registry format, but maybe we should also consider accepting upper or lowercase when reading a formatted GUID.

My intention for this patch is to fix the fatal error in the yaml2obj tool.

I would suggest to open another change request to include the option to accept upper or lowercase. But if you feel that this patch should include that change, it is fine with me.

Thanks

[amccarth]

I'm fine focusing on this part to get the bug fixed. We can consider looking at accepting upper and lower case letters in the future.

llvm/unittests/DebugInfo/CodeView/GUIDFormatTest.cpp

38

I see why it's done, but it's a bit confusing that this definition of MSGuid is different from the one in the formatting code. In fact, this one has 18 bytes of data and may contain padding.

If you were to move this definition down to the one place it's used, it might not be as confusing.

83

It's not clear to my why we need 16 of these tests. Is this overkill?

The names Data_xx don't really clue me in as to what's going on in each test set. I see the "Shifting xx" comments, but that still doesn't quite do it for me.

From the comments, I expected a set of tests for Variant 1 (UUIDs) and one for Variant 2 (MS/COM-style GUIDs).

[CarlosAlbertoEnciso]

In D103089#2791794, @amccarth wrote:

I'm fine focusing on this part to get the bug fixed. We can consider looking at accepting upper and lower case letters in the future.

Thanks for your feedback. I will address your comments and update the patch.

[CarlosAlbertoEnciso]

Address comments from @amccarth:

• Only the Variant 2 (MS/COM-style GUIDs) are used in the tests.
• Too many test cases.

llvm/unittests/DebugInfo/CodeView/GUIDFormatTest.cpp

38

I see why it's done, but it's a bit confusing that this definition of MSGuid is different from the one in the formatting code. In fact, this one has 18 bytes of data and may contain padding.

To avoid confusion between those definitions, I have renamed the one used in the tests.

If you were to move this definition down to the one place it's used, it might not be as confusing.

Moved the definition down to the place where is used.

83

It's not clear to my why we need 16 of these tests. Is this overkill?

The idea was to shift a zero value across all components. But as you pointed it out, it can be overkill. I have removed most of the tests. Keeping just one that covers the corner cases.

The names Data_xx don't really clue me in as to what's going on in each test set. I see the "Shifting xx" comments, but that still doesn't quite do it for me.

Rename the test as 'ValidateFormat'.

From the comments, I expected a set of tests for Variant 1 (UUIDs) and one for Variant 2 (MS/COM-style GUIDs).

The test are intended only for the Variant 2. Added a explanatory note.

[amccarth]

Thanks for the changes. This LGTM now.

llvm/unittests/DebugInfo/CodeView/GUIDFormatTest.cpp

94

nit: s/Shif/Shift/

[CarlosAlbertoEnciso]

@amccarth: Thanks very much for your review.

llvm/unittests/DebugInfo/CodeView/GUIDFormatTest.cpp

94

Changed 'Shif' to 'Shift'

llvm/lib/DebugInfo/CodeView/Formatters.cpp

 using namespace llvm::codeview::detail;
 
 GuidAdapter::GuidAdapter(StringRef Guid)
     : FormatAdapter(makeArrayRef(Guid.bytes_begin(), Guid.bytes_end())) {}
 
 GuidAdapter::GuidAdapter(ArrayRef<uint8_t> Guid)
     : FormatAdapter(std::move(Guid)) {}
 
+// From https://docs.microsoft.com/en-us/windows/win32/msi/guid documentation:
+// The GUID data type is a text string representing a Class identifier (ID).
+// All GUIDs must be authored in uppercase.
+// The valid format for a GUID is {XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX} where
+// X is a hex digit (0,1,2,3,4,5,6,7,8,9,A,B,C,D,E,F).
+//
+// The individual string components must be padded to comply with the specific
+// lengths of {8-4-4-4-12} characters.
+// The llvm-yaml2obj tool checks that a GUID follow that format:
+// - the total length to be 38 (including the curly braces.
+// - there is a dash at the positions: 8, 13, 18 and 23.
 void GuidAdapter::format(raw_ostream &Stream, StringRef Style) {
   assert(Item.size() == 16 && "Expected 16-byte GUID");
   struct MSGuid {
     support::ulittle32_t Data1;
     support::ulittle16_t Data2;
     support::ulittle16_t Data3;
     support::ubig64_t Data4;
   };
   const MSGuid *G = reinterpret_cast<const MSGuid *>(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 >> 48, 2, /*Upper=*/true) << '-'
-      << format_hex_no_prefix(G->Data4 & ((1ULL << 48) - 1), 6, /*Upper=*/true)
+      << '{' << format_hex_no_prefix(G->Data1, 8, /*Upper=*/true)
+      << '-' << format_hex_no_prefix(G->Data2, 4, /*Upper=*/true)
+      << '-' << format_hex_no_prefix(G->Data3, 4, /*Upper=*/true)
+      << '-' << format_hex_no_prefix(G->Data4 >> 48, 4, /*Upper=*/true) << '-'
+      << format_hex_no_prefix(G->Data4 & ((1ULL << 48) - 1), 12, /*Upper=*/true)
       << '}';
 }
 
 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/obj2yaml/COFF/test-1.test

+# RUN: obj2yaml %S/Inputs/test-1.o | yaml2obj -o %t.o
+# RUN: llvm-pdbutil dump --types %t.o | FileCheck %s -check-prefix=ALL
+
+# ALL: {{.*}} guid = {00C903AB-0968-4639-84F8-7D3E719A1BE1}

llvm/test/tools/obj2yaml/COFF/test-2.test

+# RUN: obj2yaml %S/Inputs/test-2.o | yaml2obj -o %t.o
+# RUN: llvm-pdbutil dump --types %t.o | FileCheck %s -check-prefix=ALL
+
+# ALL: {{.*}} guid = {39B59910-0D01-4889-B930-43E2FFC14694}

llvm/test/tools/obj2yaml/COFF/test-3.test

+# RUN: obj2yaml %S/Inputs/test-3.o | yaml2obj -o %t.o
+# RUN: llvm-pdbutil dump --types %t.o | FileCheck %s -check-prefix=ALL
+
+# ALL: {{.*}} guid = {A03206E7-6691-415E-A241-0B5A0D0EE37A}

llvm/test/tools/obj2yaml/COFF/test-4.test

+# RUN: obj2yaml %S/Inputs/test-4.o | yaml2obj -o %t.o
+# RUN: llvm-pdbutil dump --types %t.o | FileCheck %s -check-prefix=ALL
+
+# ALL: {{.*}} guid = {1DD12373-03EB-4522-B3D7-07B483F45F06}

llvm/test/tools/obj2yaml/COFF/test-5.test

+# RUN: obj2yaml %S/Inputs/test-5.o | yaml2obj -o %t.o
+# RUN: llvm-pdbutil dump --types %t.o | FileCheck %s -check-prefix=ALL
+
+# ALL: {{.*}} guid = {0B162ECA-2764-472E-B26B-DBDEA88F3B01}

llvm/test/tools/obj2yaml/COFF/test-6.test

+# RUN: obj2yaml %S/Inputs/test-6.o | yaml2obj -o %t.o
+# RUN: llvm-pdbutil dump --types %t.o | FileCheck %s -check-prefix=ALL
+
+# ALL: {{.*}} guid = {EA30BF0A-0A80-4424-8315-AFDD8F448DE2}

llvm/unittests/DebugInfo/CodeView/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   DebugInfoCodeView
   )
 
 add_llvm_unittest(DebugInfoCodeViewTests
+  GUIDFormatTest.cpp
   RandomAccessVisitorTest.cpp
   TypeHashingTest.cpp
   TypeIndexDiscoveryTest.cpp
   )
 
 target_link_libraries(DebugInfoCodeViewTests PRIVATE LLVMTestingSupport)

llvm/unittests/DebugInfo/CodeView/GUIDFormatTest.cpp

+//===- unittest/DebugInfo/CodeView/GUIDFormatTest.cpp - GUID formatting ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/DebugInfo/CodeView/Formatters.h"
+#include "llvm/DebugInfo/CodeView/GUID.h"
+#include "llvm/Support/FormatVariadic.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace llvm::codeview;
+
+// Variant 1 UUIDs, nowadays the most common variant, are encoded in a
+// big-endian format.
+// For example, 00112233-4455-6677-8899-aabbccddeeff is encoded as the bytes
+// 00 11 22 33 44 55 66 77 88 99 aa bb cc dd ee ff
+//
+// Variant 2 UUIDs, historically used in Microsoft's COM/OLE libraries, use a
+// mixed-endian format, whereby the first three components of the UUID are
+// little-endian, and the last two are big-endian.
+// For example, 00112233-4455-6677-8899-aabbccddeeff is encoded as the bytes
+// 33 22 11 00 55 44 77 66 88 99 aa bb cc dd ee ff.
+//
+// Note: Only Variant 2 UUIDs are tested.
+namespace {
+
+using GuidPair = std::pair<StringRef, GUID>;
+using GuidData = SmallVector<GuidPair>;
+
+void checkData(GuidData &Data) {
+  for (auto Item : Data) {
+    std::string GuidText(formatv("{0}", Item.second).str());
+    StringRef Scalar(GuidText);
+
+    // GUID strings are 38 characters long.
+    EXPECT_EQ(Scalar.size(), size_t(38));
+
+    // GUID must be enclosed in {}
+    EXPECT_EQ(Scalar.front(), '{');
+    EXPECT_EQ(Scalar.back(), '}');
+
+    Scalar = Scalar.substr(1, Scalar.size() - 2);
+    SmallVector<StringRef, 6> Component;
+    Scalar.split(Component, '-', 5);
+
+    // GUID must have 5 components.
+    EXPECT_EQ(Component.size(), size_t(5));
+
+    // GUID components are properly delineated with dashes.
+    EXPECT_EQ(Scalar[8], '-');
+    EXPECT_EQ(Scalar[13], '-');
+    EXPECT_EQ(Scalar[18], '-');
+    EXPECT_EQ(Scalar[23], '-');
+
+    // GUID only contains hex digits.
+    struct {
+      support::ulittle32_t Data0;
+      support::ulittle16_t Data1;
+      support::ulittle16_t Data2;
+      support::ubig16_t Data3;
+      support::ubig64_t Data4;
+    } G = {};
+    EXPECT_TRUE(to_integer(Component[0], G.Data0, 16));
+    EXPECT_TRUE(to_integer(Component[1], G.Data1, 16));
+    EXPECT_TRUE(to_integer(Component[2], G.Data2, 16));
+    EXPECT_TRUE(to_integer(Component[3], G.Data3, 16));
+    EXPECT_TRUE(to_integer(Component[4], G.Data4, 16));
+
+    // Check the values are the same.
+    EXPECT_EQ(Scalar, Item.first);
+  }
+}
+
+TEST(GUIDFormatTest, ValidateFormat) {
+  // Shifting 2 (0x00)
+  GuidData Data = {
+      // Non-zero values in all components.
+      {"11223344-5566-7788-99AA-BBCCDDEEFFAA",
+       {0x44, 0x33, 0x22, 0x11, 0x66, 0x55, 0x88, 0x77, 0x99, 0xaa, 0xbb, 0xcc,
+        0xdd, 0xee, 0xff, 0xaa}},
+
+      // Zero values in all components.
+      {"00000000-0000-0000-0000-000000000000",
+       {0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+        0x00, 0x00, 0x00, 0x00}},
+
+      // Shift 2 (0x00) across all components
+      {"00003344-5566-7788-99AA-BBCCDDEEFFAA",
+       {0x44, 0x33, 0x00, 0x00, 0x66, 0x55, 0x88, 0x77, 0x99, 0xaa, 0xbb, 0xcc,
+        0xdd, 0xee, 0xff, 0xaa}},
+      {"11000044-5566-7788-99AA-BBCCDDEEFFAA",
+       {0x44, 0x00, 0x00, 0x11, 0x66, 0x55, 0x88, 0x77, 0x99, 0xaa, 0xbb, 0xcc,
+        0xdd, 0xee, 0xff, 0xaa}},
+      {"11220000-5566-7788-99AA-BBCCDDEEFFAA",
+       {0x00, 0x00, 0x22, 0x11, 0x66, 0x55, 0x88, 0x77, 0x99, 0xaa, 0xbb, 0xcc,
+        0xdd, 0xee, 0xff, 0xaa}},
+      {"11223300-0066-7788-99AA-BBCCDDEEFFAA",
+       {0x00, 0x33, 0x22, 0x11, 0x66, 0x00, 0x88, 0x77, 0x99, 0xaa, 0xbb, 0xcc,
+        0xdd, 0xee, 0xff, 0xaa}},
+      {"11223344-0000-7788-99AA-BBCCDDEEFFAA",
+       {0x44, 0x33, 0x22, 0x11, 0x00, 0x00, 0x88, 0x77, 0x99, 0xaa, 0xbb, 0xcc,
+        0xdd, 0xee, 0xff, 0xaa}},
+      {"11223344-5500-0088-99AA-BBCCDDEEFFAA",
+       {0x44, 0x33, 0x22, 0x11, 0x00, 0x55, 0x88, 0x00, 0x99, 0xaa, 0xbb, 0xcc,
+        0xdd, 0xee, 0xff, 0xaa}},
+      {"11223344-5566-0000-99AA-BBCCDDEEFFAA",
+       {0x44, 0x33, 0x22, 0x11, 0x66, 0x55, 0x00, 0x00, 0x99, 0xaa, 0xbb, 0xcc,
+        0xdd, 0xee, 0xff, 0xaa}},
+      {"11223344-5566-7700-00AA-BBCCDDEEFFAA",
+       {0x44, 0x33, 0x22, 0x11, 0x66, 0x55, 0x00, 0x77, 0x00, 0xaa, 0xbb, 0xcc,
+        0xdd, 0xee, 0xff, 0xaa}},
+      {"11223344-5566-7788-0000-BBCCDDEEFFAA",
+       {0x44, 0x33, 0x22, 0x11, 0x66, 0x55, 0x88, 0x77, 0x00, 0x00, 0xbb, 0xcc,
+        0xdd, 0xee, 0xff, 0xaa}},
+      {"11223344-5566-7788-9900-00CCDDEEFFAA",
+       {0x44, 0x33, 0x22, 0x11, 0x66, 0x55, 0x88, 0x77, 0x99, 0x00, 0x00, 0xcc,
+        0xdd, 0xee, 0xff, 0xaa}},
+      {"11223344-5566-7788-99AA-0000DDEEFFAA",
+       {0x44, 0x33, 0x22, 0x11, 0x66, 0x55, 0x88, 0x77, 0x99, 0xaa, 0x00, 0x00,
+        0xdd, 0xee, 0xff, 0xaa}},
+      {"11223344-5566-7788-99AA-BB0000EEFFAA",
+       {0x44, 0x33, 0x22, 0x11, 0x66, 0x55, 0x88, 0x77, 0x99, 0xaa, 0xbb, 0x00,
+        0x00, 0xee, 0xff, 0xaa}},
+      {"11223344-5566-7788-99AA-BBCC0000FFAA",
+       {0x44, 0x33, 0x22, 0x11, 0x66, 0x55, 0x88, 0x77, 0x99, 0xaa, 0xbb, 0xcc,
+        0x00, 0x00, 0xff, 0xaa}},
+      {"11223344-5566-7788-99AA-BBCCDD0000AA",
+       {0x44, 0x33, 0x22, 0x11, 0x66, 0x55, 0x88, 0x77, 0x99, 0xaa, 0xbb, 0xcc,
+        0xdd, 0x00, 0x00, 0xaa}},
+      {"11223344-5566-7788-99AA-BBCCDDEE0000",
+       {0x44, 0x33, 0x22, 0x11, 0x66, 0x55, 0x88, 0x77, 0x99, 0xaa, 0xbb, 0xcc,
+        0xdd, 0xee, 0x00, 0x00}},
+  };
+
+  checkData(Data);
+}
+} // namespace