Add new printNumber() for size_t

[junhee-yoo]

Summary

Add printNumber(StringRef Label, size_t Value) to fix ambiguous problem.

[junhee-yoo]

After https://reviews.llvm.org/D137096, I faced compile error:

/llvm-project/llvm/tools/llvm-readobj/ELFDumper.cpp:7182:7: error: call to member function 'printNumber' is ambiguous
    W.printNumber("Count", Count[I]);
    ~~^~~~~~~~~~~
               ^
12 errors generated

which was occurred by absence of "printNumber(StringRef Label, size_t Value)".
To fix this, I just added that one.

Env:
OS: macOS 13.2.1 (22D68)
LLVM version: 14.0.6 (from homebrew)

Build command:

CMake:

cmake -DLLVM_TARGETS_TO_BUILD="AArch64" -DCMAKE_BUILD_TYPE=Debug -DLLVM_ENABLE_ASSERTIONS=ON -DLLVM_BUILD_EXAMPLES=ON -DLLVM_ENABLE_PROJECTS=mlir -G Ninja ../llvm

Compile command:

/opt/homebrew/opt/llvm@14/bin/clang++ -DBUILD_EXAMPLES -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GLIBCXX_ASSERTIONS -D_LIBCPP_ENABLE_ASSERTIONS -DSTDC_CONSTANT_MACROS -DSTDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I<path-to>/llvm-project/build/tools/llvm-readobj -I<path-to>/llvm-project/llvm/tools/llvm-readobj -I<path-to>/llvm-project/build/include -I<path-to>/llvm-project/llvm/include -isystem /opt/homebrew/include -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -g -arch arm64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.1.sdk -fno-exceptions -funwind-tables -fno-rtti -std=c++17 -MD -MT tools/llvm-readobj/CMakeFiles/llvm-readobj.dir/ELFDumper.cpp.o -MF tools/llvm-readobj/CMakeFiles/llvm-readobj.dir/ELFDumper.cpp.o.d -o tools/llvm-readobj/CMakeFiles/llvm-readobj.dir/ELFDumper.cpp.o -c <path-to>/llvm-project/llvm/tools/llvm-readobj/ELFDumper.cpp

• hide some information with <path-to>

Full compile error message:

/llvm-project/llvm/tools/llvm-readobj/ELFDumper.cpp:7182:7: error: call to member function 'printNumber' is ambiguous

W.printNumber("Count", Count[I]);
~~^~~~~~~~~~~

/llvm-project/llvm/include/llvm/Support/ScopedPrinter.h:201:16: note: candidate function

virtual void printNumber(StringRef Label, uint64_t Value) {
             ^

/llvm-project/llvm/include/llvm/Support/ScopedPrinter.h:205:16: note: candidate function

virtual void printNumber(StringRef Label, uint32_t Value) {
             ^

/llvm-project/llvm/include/llvm/Support/ScopedPrinter.h:209:16: note: candidate function

virtual void printNumber(StringRef Label, uint16_t Value) {
             ^

/llvm-project/llvm/include/llvm/Support/ScopedPrinter.h:213:16: note: candidate function

virtual void printNumber(StringRef Label, uint8_t Value) {
             ^

/llvm-project/llvm/include/llvm/Support/ScopedPrinter.h:217:16: note: candidate function

virtual void printNumber(StringRef Label, int64_t Value) {
             ^

/llvm-project/llvm/include/llvm/Support/ScopedPrinter.h:221:16: note: candidate function

virtual void printNumber(StringRef Label, int32_t Value) {
             ^

/llvm-project/llvm/include/llvm/Support/ScopedPrinter.h:225:16: note: candidate function

virtual void printNumber(StringRef Label, int16_t Value) {
             ^

/llvm-project/llvm/include/llvm/Support/ScopedPrinter.h:229:16: note: candidate function

virtual void printNumber(StringRef Label, int8_t Value) {
             ^

/llvm-project/llvm/include/llvm/Support/ScopedPrinter.h:237:16: note: candidate function

virtual void printNumber(StringRef Label, float Value) {
             ^

/llvm-project/llvm/include/llvm/Support/ScopedPrinter.h:241:16: note: candidate function

  virtual void printNumber(StringRef Label, double Value) {
               ^
12 errors generated.
[3837/4064] Building CXX object tools/llvm-reduce/CMakeFiles/llvm-reduce.dir/ReducerWorkItem.cpp.o
ninja: build stopped: subcommand failed.

[paulkirth]

Hi, sorry this is causing you a problem. I'm surprised our builders haven't picked this up. I'm also unable to reproduce this locally on a Mac or Linux machine.

Right now, it looks like CI also is running into trouble with this patch, so I'm not sure the additional overload is a good solution.

As a quick check, can you try casting the value instead? Something along these lines:

W.printNumber("Count", static_cast<uint64_t>(Count[I]));

If that works, we can probably change the container type away from ArrayRef<size_t> to use uint64_t or maybe even uint32_t.

[junhee-yoo]

@paulkirth thanks for your kind reply.

I've checked static_cast<uint64_t> is worked, but few more lines need to be applied.

--- a/llvm/tools/llvm-readobj/ELFDumper.cpp
+++ b/llvm/tools/llvm-readobj/ELFDumper.cpp
@@ -7172,14 +7172,14 @@ void LLVMELFDumper<ELFT>::printHashHistogramStats(size_t NBucket,
   StringRef BucketName = IsGnu ? "Bucket" : "Chain";
   StringRef ListName = IsGnu ? "Buckets" : "Chains";
   DictScope Outer(W, HistName);
-  W.printNumber("TotalBuckets", NBucket);
+  W.printNumber("TotalBuckets", static_cast<uint64_t>(NBucket));
   ListScope Buckets(W, ListName);
   size_t CumulativeNonZero = 0;
   for (size_t I = 0; I < MaxChain; ++I) {
     CumulativeNonZero += Count[I] * I;
     DictScope Bucket(W, BucketName);
-    W.printNumber("Length", I);
-    W.printNumber("Count", Count[I]);
+    W.printNumber("Length", static_cast<uint64_t>(I));
+    W.printNumber("Count", static_cast<uint64_t>(Count[I]));
     W.printNumber("Percentage", (float)(Count[I] * 100.0) / NBucket);
     W.printNumber("Coverage", (float)(CumulativeNonZero * 100.0) / TotalSyms);
   }

I'm not sure the reason why it reproduced on my labtop - I'd also tried for clean build - but it happens everytimes.
I also saw the build error log on CI/CD and it seems that the compiler of CI/CD treats size_t as uint64_t.
And I found that there is -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.1.sdk on the compile command.

Please let me try to build this again only within llvm@14.

PS. Sorry to make your mailbox massy with my summary editing.

[jhenderson]

My understanding is that size_t isn't a fundamental type, and it shouldn't need to have its own overload. @junhee-yoo, to help us understand why the ambiguity occurs, I think we need to know what type your system's size_t actually is. For example, if I go to the definition of size_t on my Windows host, I find that it is defined as: typedef unsigned __int64 size_t;, which is synonymous with unsigned long long, the same type as what uint64_t is a typedef of. Before blindly adding a size_t overload, I think we need to find out what your size_t is actually defined as on your system. You'll need to dig through your system headers to find where the size_t type is defined (there may be a more effective way of doing this through an IDE, but I'm not familiar with Mac systems, so can't advise you on this).

As you have observed, your change actually causes build failures on the pre-merge builds. If you take a look at the Windows report (https://buildkite.com/llvm-project/premerge-checks/builds/142201#01870203-5f63-451f-9b53-30b0614ebda9), you will see that there's a compilation failure as follows:

C:\ws\w5\llvm-project\premerge-checks\llvm\include\llvm/Support/ScopedPrinter.h(205): error C2535: 'void llvm::ScopedPrinter::printNumber(llvm::StringRef,size_t)': member function already defined or declared
C:\ws\w5\llvm-project\premerge-checks\llvm\include\llvm/Support/ScopedPrinter.h(201): note: see declaration of 'llvm::ScopedPrinter::printNumber'

You'll observe that the two lines referenced refer to the size_t and uint64_t overloads. This is because the two have the same fundamental type and therefore it's equivalent to you writing 'void llvm::ScopedPrinter::printNumber(llvm::StringRef,unsigned long long)' twice. The Linux pre-merge check produces essentially the same error for the same reason.

[paulkirth]

@junhee-yoo can you try https://reviews.llvm.org/D146544? I think that may be a good solution here, so long as @jhenderson agrees it won't run into problems w/ overflow.

[junhee-yoo]

@jhenderson Thanks for your kind comment.

From your comment, I've inspected each alias types uint64_t and size_t in Windows(by reference), Linux(by IDE) and macOS(by IDE).
After this, I found that macOS has different base type for uint64_t and size_t but their size is same.

	uint64_t	size_t
Windows, Ref	unsigned long long	unsigned long long
Linux (x86_64, ubuntu 22.04, Ubuntu clang version 14.0.0-1ubuntu1)	unsigned long, size: 8	unsigned long, size: 8
macOS (M1 Pro(aarch64), MacOSX13.1.sdk + Homebrew clang version 14.0.6)	unsigned long long, size: 8	unsigned long, size: 8

I guess this is my compile error comes from. I checked this with a small experiment on my macOS:

#include <iostream>

void print(uint64_t a) {
    std::cout << a << std::endl;
}

void print(int64_t a) {
    std::cout << a << std::endl;
}

int main()
{
    size_t a = 10;
    print(a);  // error: call to 'print' is ambiguous

    return 0;
}

I thought if I use only homebrew llvm@14, this ambiguous problem could be solved, but I couldn't remove or properly change my isysroot arguments from my CMake configure.
(I also changed CMAKE_OSX_SYSROOT in my CMake config, it occurs include header not found error)
So, instead of it, I've tried to compile above experiment C++ code with homebrew clang and results is same: error: call to 'print' is ambiguous.

[junhee-yoo]

In D146492#4210239, @paulkirth wrote:

@junhee-yoo can you try https://reviews.llvm.org/D146544? I think that may be a good solution here, so long as @jhenderson agrees it won't run into problems w/ overflow.

I'd checked your change and I think it will work for ELFDumper.cc but in my humble opinion, calling printNumber() with size_t still have a potential problem who doesn't know about.

By the way, @paulkirth if you mind, would you tell me your Mac build env and command to me for reproducing success build? If this problem can be solved by build config, Maybe the proper way to solve this is changing build configs rather than changing the code.

In D146492#4208618, @paulkirth wrote:

Hi, sorry this is causing you a problem. I'm surprised our builders haven't picked this up. I'm also unable to reproduce this locally on a Mac or Linux machine.

[paulkirth]

@junhee-yoo you may want to try your code w/ top-of-tree again. looks like someone added some static_cast to those APIs earlier today, so you may no longer have an issue. I didn't even notice that had landed earlier when I proposed changing to uint32_t.

[paulkirth]

In D146492#4211751, @junhee-yoo wrote:

In D146492#4210239, @paulkirth wrote:

@junhee-yoo can you try https://reviews.llvm.org/D146544? I think that may be a good solution here, so long as @jhenderson agrees it won't run into problems w/ overflow.

I'd checked your change and I think it will work for ELFDumper.cc but in my humble opinion, calling printNumber() with size_t still have a potential problem who doesn't know about.

By the way, @paulkirth if you mind, would you tell me your Mac build env and command to me for reproducing success build? If this problem can be solved by build config, Maybe the proper way to solve this is changing build configs rather than changing the code.

In D146492#4208618, @paulkirth wrote:

Hi, sorry this is causing you a problem. I'm surprised our builders haven't picked this up. I'm also unable to reproduce this locally on a Mac or Linux machine.

$ clang --version
Apple clang version 14.0.0 (clang-1400.0.29.202)
Target: x86_64-apple-darwin22.3.0

I also tried w/ ToT clang from yesterday that I had built locally.

[paulkirth]

CMake:

cmake -G Ninja -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=ON -DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra;lld" -DLLVM_ENABLE_RUNTIMES="compiler-rt" ~/llvm-project/llvm -DBUILTINS_CMAKE_ARGS=-DCOMPILER_RT_ENABLE_IOS=OFF

[junhee-yoo]

In D146492#4211754, @paulkirth wrote:

@junhee-yoo you may want to try your code w/ top-of-tree again. looks like someone added some static_cast to those APIs earlier today, so you may no longer have an issue. I didn't even notice that had landed earlier when I proposed changing to uint32_t.

Thanks for sharing. 👍 https://reviews.llvm.org/rG064e2497e2ebe9ac30ac96923a26a52484300fdf it looks same as your first suggestion and it has been finished successfully with the config that you told me.

Meanwhile, it seems your Mac CPU (x86_64) and my one(M1: aarch64) are different so it makes different results.

Homebrew clang version 14.0.6
Target: arm64-apple-darwin22.3.0
Thread model: posix
InstalledDir: /opt/homebrew/opt/llvm@14/bin

I guess this ambiguity problem only happens in M1(aarch64). I've tried to build a parent of https://reviews.llvm.org/rG064e2497e2ebe9ac30ac96923a26a52484300fdf with your config to confirm my assumption and ambiguous problem occurred.

[jhenderson]

Thanks @junhee-yoo, the difference that Mac uses unsigned long not unsigned long long was the piece I was missing. I bet if you look at the uint32_t type you'll see it is defined as unsigned int. This will mean that the current printNumber overloads for Mac probably cover the following fundamental integer types:

• char (or possibly signed char) (from int8_t)
• short (from int16_t)
• int (from int32_t)
• long long (from int64_t)

and their unsigned equivalents from the uint*_t types.

The problem ultimately boils down to the fact that we've attempted to cover all sizes of integers with printNumber overloads, but haven't actually done so, because we've used the fixed-width integer types, rather than the fundamental C++ types. The latter are what are ultimately used in picking which function overload to use. I'd therefore like to propose the following changes:

1. Replace the existing printNumber integer type overloads with fundamental type versions (specifically char, signed char, unsigned char, short, unsigned short, int, unsigned int, long, unsigned long, long long, and unsigned long long (see https://en.cppreference.com/w/cpp/language/types for a good reference).
2. Revert any "fixes" (casts etc) that have been added to make the code work on Mac or other platforms, since they'll be unnecessary at this point and just clutter the code (and none of them were reviewed...).

I'd do this myself, but actually coding on LLVM falls outside my job remit these days. @paulkirth, it probably should be your responsibility as the person who originally introduced the breakage to coordinate this!

[paulkirth]

@jhenderson sure, I can set aside some time for that. I'm a bit under the weather, so it may take me a day or so to get to it, though.

llvm/include/llvm/Support/ScopedPrinter.h

while (Curr > 0) {
       if (Curr & 1)
         SetFlags.emplace_back(Flag);
       Curr >>= 1;
       Flag <<= 1;
     }
     printFlagsImpl(Label, hex(Value), SetFlags);
   }
 
+  virtual void printNumber(StringRef Label, size_t Value) {
+    startLine() << Label << ": " << Value << "\n";
+  }
+
   virtual void printNumber(StringRef Label, uint64_t Value) {
     startLine() << Label << ": " << Value << "\n";
   }
 
   virtual void printNumber(StringRef Label, uint32_t Value) {
     startLine() << Label << ": " << Value << "\n";
   }