Convert float to double on __builtin_dump_struct

[rafaelfranco]

Summary

Variadic arguments of float type are automatically promoted to double.
This commit makes the __builtin_dump_struct to convert float arguments
to double before passing them to the printf-style function, fixing this
bug https://bugs.llvm.org/show_bug.cgi?id=45143.

[rafaelfranco]

This seems to fix the problem. Feedback etc. more than welcome :)

[rafaelfranco]

I have added John McCall to the reviewers as the CODE_OWNERS.txt file says he's responsible for LLVM IR generation. Also Aaron Ballman, as he wrote the function per git-blame output.

[aaron.ballman]

Adding @paulsemel to the reviewer list as he was the original author of this functionality (I commit on his behalf which is how I showed up on the git blame).

clang/lib/CodeGen/CGBuiltin.cpp

2090–2094

This change is an improvement as far as it goes, but I think we might be missing other floating-point promotions here. For example, __fp16 fields also seem to be unusable: https://godbolt.org/z/z3a45f9YE

Also, we don't seem to handle the integer promotions at all (but still get correct results there), so I think we're getting the correct behavior there by chance rather than by design. Oh, yeah, note the differences here: https://godbolt.org/z/f13eq3668

foo:
  ...
  %7 = load i8, i8* %4, align 1, !dbg !217
  %8 = call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([6 x i8], [6 x i8]* @2, i32 0, i32 0), i8 %7), !dbg !217
  ...

bar:
  ...
  %2 = load i8, i8* %1, align 1, !dbg !222
  %3 = zext i8 %2 to i32, !dbg !222
  %4 = call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([5 x i8], [5 x i8]* @.str, i64 0, i64 0), i32 %3), !dbg !223
  ...

I think we should probably fix all of the promotion problems at once rather than piecemeal.

[rjmccall]

clang/lib/CodeGen/CGBuiltin.cpp

2090–2094

It's actually really annoying that this logic has to be duplicated in IRGen instead of being able to take advantage of the existing promotion logic in Sema. Can we just generate a helper function in Sema and somehow link it to the builtin call?

Um. Also, the static local DenseMap in the code above this is totally unacceptable and should not have been committed. Clang is supposed to be embeddable as a library and should not be using global mutable variables.

[aaron.ballman]

clang/lib/CodeGen/CGBuiltin.cpp

2090–2094

It's actually really annoying that this logic has to be duplicated in IRGen instead of being able to take advantage of the existing promotion logic in Sema. Can we just generate a helper function in Sema and somehow link it to the builtin call?

That seems worth a shot but I think it is not something that needs to happen for this patch (that could be a rather heavy lift to ask someone who just joined the community) unless the basic fix starts getting out of hand.

Um. Also, the static local DenseMap in the code above this is totally unacceptable and should not have been committed. Clang is supposed to be embeddable as a library and should not be using global mutable variables.

We do that with some degree of frequency already (command line arguments using llvm::cl::opt are all global mutable variables, such as https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CoverageMappingGen.cpp#L34), but yeah, fixing that up would also not be a bad idea, but orthogonal to this patch IMO.

[rjmccall]

clang/lib/CodeGen/CGBuiltin.cpp

2090–2094

We do that with some degree of frequency already (command line arguments using llvm:🆑:opt are all global mutable variables, such as https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CoverageMappingGen.cpp#L34), but yeah, fixing that up would also not be a bad idea, but orthogonal to this patch IMO.

That should also not have been committed, but more importantly, it's not actually mutated during normal operation — IIUC, you have to tell the cl library to process a command line for any of the cl::opts to be mutated, and otherwise they remain constant. In contrast, this code may segfault if you have two threads in a process running IR-generation that happen to use this builtin, and the only saving grace is that approximately nobody uses this builtin.

2090–2094

That seems worth a shot but I think it is not something that needs to happen for this patch (that could be a rather heavy lift to ask someone who just joined the community) unless the basic fix starts getting out of hand.

In addition to its need to reproduce all the logic of default argument conversion, this code is doing manual lowering of calls with arbitrary arguments instead of using the target ABI call-emission code. It also, as mentioned in the FIXME, doesn't properly handle bit-fields. It's just the wrong basic approach for implementing this feature, and I don't think we should be trying to clean it up around the edges; we should revert and ask for an acceptable implementation.

[rafaelfranco]

Hey all! Thanks for taking the time to review my patch and writing the Compiler Explorer examples and everything. I had no idea this was the essentially the wrong approach to this, I'd be happy to do a bigger overhaul of the whole builtin if that would make it more correct, but as Aaron points out I'm very new to this project (and C++ too) and essentially clueless as to how to do that, I submitted this patch because it looked like it was simple enough to issue the fpext to get the float promoted.
If you give me some pointers I'd be more than happy to give it a shot, I should have time in the coming weeks. As a seasoned printf debugger 😄 this builtin is pretty useful to me and I'd like to fix it rather than deprecating or otherwise removing it.
As for the static map, it looks to me like it would be fairly straightforward to replace it with a simple helper function?

[rjmccall]

In D112626#3167520, @rafaelfranco wrote:

Hey all! Thanks for taking the time to review my patch and writing the Compiler Explorer examples and everything. I had no idea this was the essentially the wrong approach to this, I'd be happy to do a bigger overhaul of the whole builtin if that would make it more correct, but as Aaron points out I'm very new to this project (and C++ too) and essentially clueless as to how to do that, I submitted this patch because it looked like it was simple enough to issue the fpext to get the float promoted.
If you give me some pointers I'd be more than happy to give it a shot, I should have time in the coming weeks. As a seasoned printf debugger 😄 this builtin is pretty useful to me and I'd like to fix it rather than deprecating or otherwise removing it.

Thanks for this. I'd be happy to help you through it. And yeah, I'm definitely not arguing for deprecating/removing the builtin long-term; I just want the implementation to be on a sound technical footing.

As for the static map, it looks to me like it would be fairly straightforward to replace it with a simple helper function?

Yeah, a helper function that just returns a format specifier for a type seems like the way to go, and that would be a reasonable short-term patch. When this code moves into Sema, hopefully we can find a way to merge that with the logic we already have for printf checking.

clang/lib/CodeGen/CGBuiltin.cpp

for (const auto *FD : RD->fields()) {
     // We try to determine the best format to print the current field
     llvm::Twine Format = Types.find(CanonicalType) == Types.end()
                              ? Types[Context.VoidPtrTy]
                              : Types[CanonicalType];
 
     Address FieldAddress = Address(FieldPtr, Align);
     FieldPtr = CGF.Builder.CreateLoad(FieldAddress);
 
+    // Variadic functions expect the caller to promote float to double.
+    if (CanonicalType == Context.FloatTy) {
+      FieldPtr =
+          CGF.Builder.CreateFPExt(FieldPtr, CGF.ConvertType(Context.DoubleTy));
+    }
+
     // FIXME Need to handle bitfield here
     GString = CGF.Builder.CreateGlobalStringPtr(
         Format.concat(llvm::Twine('\n')).str());
     TmpRes = CGF.Builder.CreateCall(Func, {GString, FieldPtr});
     Res = CGF.Builder.CreateAdd(Res, TmpRes);
   }
 
   GString = CGF.Builder.CreateGlobalStringPtr(Pad + "}\n");

clang/test/CodeGen/dump-struct-builtin.c

 // CHECK-NEXT: [[END_STRUCT_U17:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00"
 
 // CHECK: @__const.unit18.a = private unnamed_addr constant %struct.U18A { x86_fp80 0xK3FFF8FCD67FD3F5B6000 }, align 16
 // CHECK-NEXT: [[STRUCT_STR_U18:@[0-9]+]] = private unnamed_addr constant [15 x i8] c"struct U18A {\0A\00"
 // CHECK-NEXT: [[FIELD_U18:@[0-9]+]] = private unnamed_addr constant [17 x i8] c"long double a : \00"
 // CHECK-NEXT: [[FORMAT_U18:@[0-9]+]] = private unnamed_addr constant [5 x i8] c"%Lf\0A\00"
 // CHECK-NEXT: [[END_STRUCT_U18:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00"
 
+// CHECK: @__const.unit19.a = private unnamed_addr constant %struct.U19A { float 0x3FF1F9AD00000000 }, align 4
+// CHECK-NEXT: [[STRUCT_STR_U19:@[0-9]+]] = private unnamed_addr constant [15 x i8] c"struct U19A {\0A\00"
+// CHECK-NEXT: [[FIELD_U19:@[0-9]+]] = private unnamed_addr constant [11 x i8] c"float a : \00"
+// CHECK-NEXT: [[FORMAT_U19:@[0-9]+]] = private unnamed_addr constant [4 x i8] c"%f\0A\00"
+// CHECK-NEXT: [[END_STRUCT_U19:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00"
+
 int printf(const char *fmt, ...) {
     return 0;
 }
 
 void unit1() {
   struct U1A {
     short a;
   };
void test4() {
 
   // CHECK: [[BC2:%[0-9]+]] = bitcast %union.anon.0* [[RES1]] to %struct.anon.1*
   // CHECK: [[RES3:%[0-9]+]] = getelementptr inbounds %struct.anon.1, %struct.anon.1* [[BC2]], i32 0, i32 0
   // CHECK: [[LOAD2:%[0-9]+]] = load i64, i64* [[RES3]],
   // CHECK: call i32 (i8*, ...) @printf({{.*}}, i64 [[LOAD2]])
   // CHECK: call i32 (i8*, ...) @printf(
   __builtin_dump_struct(&a, &printf);
 }
+
+void unit19() {
+  struct U19A {
+    float a;
+  };
+
+  struct U19A a = {
+      .a = 1.123456f,
+  };
+
+  // CHECK: call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([15 x i8], [15 x i8]* [[STRUCT_STR_U19]], i32 0, i32 0))
+  // CHECK: [[RES1:%[0-9]+]] = getelementptr inbounds %struct.U19A, %struct.U19A* %a, i32 0, i32 0
+  // CHECK: call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([11 x i8], [11 x i8]* [[FIELD_U19]], i32 0, i32 0))
+  // CHECK: [[LOAD1:%[0-9]+]] = load float, float* [[RES1]],
+  // CHECK: [[FPEXT1:%[0-9]+]] = fpext float [[LOAD1]] to double
+  // CHECK: call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([4 x i8], [4 x i8]* [[FORMAT_U19]], i32 0, i32 0), double [[FPEXT1]])
+  // CHECK: call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([3 x i8], [3 x i8]* [[END_STRUCT_U19]], i32 0, i32 0)
+  __builtin_dump_struct(&a, &printf);
+}