Fix incorrect formatting of DataRefImpl members in operator<< function

[davidb]

Summary

Changed format specifiers to use format macro constant for pointer type.
Moved width part of format specifier in the correct place for formatting members a and b.

Added a unit test to confirm the output.

[davidb]

ping

[samparker]

LGTM

[filcab]

include/llvm/Object/SymbolicFile.h

35

Please don't add whitespace.

38

Shouldn't it be "...%" PRIxPTR?
Why change the way it dumps pointers, though? Even for uint32_t, we're dumping as 8B. Why not always dump as 8B and be done with it?

[filcab]

include/llvm/Object/SymbolicFile.h

38

Since you're changing this line anyway, can you clang-format it?

[davidb]

include/llvm/Object/SymbolicFile.h

38

Why change the way it dumps pointers, though? Even for uint32_t, we're dumping as 8B. Why not always dump as 8B and be done with it?

Sorry, not entirely sure I understand what you are asking. I changed it because it was unable to dump a 64-bit pointer.

I intentionally tried to keep the format as close as the original intent appeared to be (minimum width of 8 characters to represent the full 32-bit field). In the case that p > 32-bit value, then the character length will of course expand....

[filcab]

include/llvm/Object/SymbolicFile.h

38

Nevermind my 8B stuff. I was thinking in terms of bytes, not in terms of nibbles (which will be the width here).

include/llvm/Object/SymbolicFile.h

 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_OBJECT_SYMBOLICFILE_H
 #define LLVM_OBJECT_SYMBOLICFILE_H
 
 #include "llvm/Object/Binary.h"
 #include "llvm/Support/Format.h"
+#include <cinttypes>
 #include <utility>
 
 namespace llvm {
 namespace object {
 
 union DataRefImpl {
   // This entire union should probably be a
   // char[max(8, sizeof(uintptr_t))] and require the impl to cast.
   struct {
     uint32_t a, b;
   } d;
   uintptr_t p;
   DataRefImpl() { std::memset(this, 0, sizeof(DataRefImpl)); }
 };
 
+
 template <typename OStream>
 OStream& operator<<(OStream &OS, const DataRefImpl &D) {
-  OS << "(" << format("0x%x8", D.p) << " (" << format("0x%x8", D.d.a) << ", " << format("0x%x8", D.d.b) << "))";
+  OS << "(" << format("0x%08x" PRIxPTR, D.p) << " (" << format("0x%08x", D.d.a) << ", " << format("0x%08x", D.d.b) << "))";
   return OS;
 }
 
 inline bool operator==(const DataRefImpl &a, const DataRefImpl &b) {
   // Check bitwise identical. This is the only legal way to compare a union w/o
   // knowing which member is in use.
   return std::memcmp(&a, &b, sizeof(DataRefImpl)) == 0;
 }

unittests/Object/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Object
   )
 
 add_llvm_unittest(ObjectTests
   SymbolSizeTest.cpp
+  SymbolicFileTest.cpp
   )
 

unittests/Object/SymbolicFileTest.cpp

+//===- SymbolicFileTest.cpp - Tests for SymbolicFile.cpp ------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/Object/SymbolicFile.h"
+#include "llvm/Support/raw_ostream.h"
+#include "gtest/gtest.h"
+#include <sstream>
+
+TEST(Object, DataRefImplOstream) {
+  std::string s;
+  llvm::raw_string_ostream OS(s);
+  llvm::object::DataRefImpl Data;
+  Data.d.a = 0xeeee0000;
+  Data.d.b = 0x0000ffff;
+
+  static_assert(sizeof Data.p == sizeof(uint64_t) ||
+                    sizeof Data.p == sizeof(uint32_t),
+                "Test expected pointer type to be 32 or 64-bit.");
+
+  char const *Expected = sizeof Data.p == sizeof(uint64_t)
+                             ? "(0xffffeeee0000 (0xeeee0000, 0x0000ffff))"
+                             : "(0xeeee0000 (0xeeee0000, 0x0000ffff))";
+
+  OS << Data;
+  OS.flush();
+
+  EXPECT_EQ(Expected, s);
+}