This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by aorlov on Apr 6 2021, 10:52 AM.

Diff Detail

Event Timeline

aorlov created this revision.Apr 6 2021, 10:52 AM
aorlov requested review of this revision.Apr 6 2021, 10:52 AM
aganea added inline comments.Apr 6 2021, 11:52 AM
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)
         << "}";
}
aorlov updated this revision to Diff 335690.Apr 6 2021, 6:21 PM
aorlov updated this revision to Diff 335824.Apr 7 2021, 8:38 AM
rnk added inline comments.Apr 7 2021, 10:36 AM
llvm/lib/DebugInfo/CodeView/Formatters.cpp
28

+1

aorlov updated this revision to Diff 335938.Apr 7 2021, 3:30 PM
aorlov marked 2 inline comments as done.
aorlov updated this revision to Diff 336112.Apr 8 2021, 8:10 AM
aorlov updated this revision to Diff 336116.Apr 8 2021, 8:19 AM
Harbormaster completed remote builds in B97740: Diff 336116.
aganea added a comment.Apr 8 2021, 9:30 AM

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;
lebedev.ri retitled this revision from Fixed CodeView GuidAdapter::format to handle GUID bytes in the right order. to [lld] Fixed CodeView GuidAdapter::format to handle GUID bytes in the right order..Apr 8 2021, 9:32 AM
aorlov updated this revision to Diff 336251.Apr 8 2021, 4:03 PM
aorlov marked an inline comment as done.Apr 8 2021, 4:08 PM
aorlov added inline comments.
llvm/lib/ObjectYAML/CodeViewYAMLTypes.cpp
163 ↗(On Diff #336116)

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

aorlov marked an inline comment as done.Apr 8 2021, 4:10 PM
aganea accepted this revision.Apr 8 2021, 5:44 PM

LGTM. Thank you!

This revision is now accepted and ready to land.Apr 8 2021, 5:44 PM