[PATCH] D44093: [BUILTINS] structure pretty printer
aaron.ballman added a comment. In https://reviews.llvm.org/D44093#1063658, @paulsemel wrote: > Sorry about it, I also have the warning on my machine, but not the error you > get... > Those test are actually working on my different linux machines, that's > really weird. > This one is actually really weird, because I could find manually the missing > pattern in your output.. I just don't get what is happening. I think the issue has to do with bit-width. From unit5(): // Your test // CHECK: call i32 (i8*, ...) @printf( // CHECK: [[RES1:%[0-9]+]] = getelementptr inbounds %struct.U5A, %struct.U5A* %a, i32 0, i32 0 // CHECK: call i32 (i8*, ...) @printf( // CHECK: [[LOAD1:%[0-9]+]] = load i64, i64* [[RES1]], // CHECK: call i32 (i8*, ...) @printf({{.*}}, i64 [[LOAD1]]) // CHECK: call i32 (i8*, ...) @printf( // My results ; Function Attrs: noinline nounwind optnone define dso_local void @unit5() #0 { entry: %a = alloca %struct.U5A, align 4 %0 = bitcast %struct.U5A* %a to i8* call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 4 %0, i8* align 4 bitcast (%struct.U5A* @unit5.a to i8*), i64 4, i1 false) %1 = call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([14 x i8], [14 x i8]* @16, i32 0, i32 0)) %2 = getelementptr inbounds %struct.U5A, %struct.U5A* %a, i32 0, i32 0 %3 = call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([10 x i8], [10 x i8]* @17, i32 0, i32 0)) %4 = add i32 %1, %3 %5 = load i32, i32* %2, align 4 %6 = call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([5 x i8], [5 x i8]* @18, i32 0, i32 0), i32 %5) %7 = add i32 %4, %6 %8 = call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([3 x i8], [3 x i8]* @19, i32 0, i32 0)) %9 = add i32 %7, %8 ret void } You are testing that the load is an i64 but in my results it's an i32. An explicit target triple on the RUN line solves the issue. Repository: rC Clang https://reviews.llvm.org/D44093 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44093: [BUILTINS] structure pretty printer
paulsemel updated this revision to Diff 141920. paulsemel added a comment. Added triple option to CodeGen test so that it matches with the correct architecture Repository: rC Clang https://reviews.llvm.org/D44093 Files: include/clang/Basic/Builtins.def lib/CodeGen/CGBuiltin.cpp lib/Sema/SemaChecking.cpp test/CodeGen/dump-struct-builtin.c test/Sema/builtin-dump-struct.c Index: test/Sema/builtin-dump-struct.c === --- /dev/null +++ test/Sema/builtin-dump-struct.c @@ -0,0 +1,42 @@ +// RUN: %clang_cc1 -triple i386-unknown-unknown -fsyntax-only -fno-spell-checking -verify %s + +void invalid_uses() { + struct A { + }; + struct A a; + void *b; + int (*goodfunc)(const char *, ...); + int (*badfunc1)(const char *); + int (*badfunc2)(int, ...); + void (*badfunc3)(const char *, ...); + int (*badfunc4)(char *, ...); + int (*badfunc5)(void); + + __builtin_dump_struct(); // expected-error {{too few arguments to function call, expected 2, have 0}} + __builtin_dump_struct(1);// expected-error {{too few arguments to function call, expected 2, have 1}} + __builtin_dump_struct(1, 2); // expected-error {{passing 'int' to parameter of incompatible type structure pointer: type mismatch at 1st parameter ('int' vs structure pointer)}} + __builtin_dump_struct(, 2);// expected-error {{passing 'int' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int' vs 'int (*)(const char *, ...)')}} + __builtin_dump_struct(b, goodfunc); // expected-error {{passing 'void *' to parameter of incompatible type structure pointer: type mismatch at 1st parameter ('void *' vs structure pointer)}} + __builtin_dump_struct(, badfunc1); // expected-error {{passing 'int (*)(const char *)' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int (*)(const char *)' vs 'int (*)(const char *, ...)')}} + __builtin_dump_struct(, badfunc2); // expected-error {{passing 'int (*)(int, ...)' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int (*)(int, ...)' vs 'int (*)(const char *, ...)')}} + __builtin_dump_struct(, badfunc3); // expected-error {{passing 'void (*)(const char *, ...)' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('void (*)(const char *, ...)' vs 'int (*)(const char *, ...)')}} + __builtin_dump_struct(, badfunc4); // expected-error {{passing 'int (*)(char *, ...)' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int (*)(char *, ...)' vs 'int (*)(const char *, ...)')}} + __builtin_dump_struct(, badfunc5); // expected-error {{passing 'int (*)(void)' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int (*)(void)' vs 'int (*)(const char *, ...)')}} + __builtin_dump_struct(a, goodfunc); // expected-error {{passing 'struct A' to parameter of incompatible type structure pointer: type mismatch at 1st parameter ('struct A' vs structure pointer)}} +} + +void valid_uses() { + struct A { + }; + union B { + }; + + int (*goodfunc)(const char *, ...); + int (*goodfunc2)(); + struct A a; + union B b; + + __builtin_dump_struct(, goodfunc); + __builtin_dump_struct(, goodfunc); + __builtin_dump_struct(, goodfunc2); +} Index: test/CodeGen/dump-struct-builtin.c === --- /dev/null +++ test/CodeGen/dump-struct-builtin.c @@ -0,0 +1,391 @@ +// RUN: %clang_cc1 -triple=x86_64-unknown-linux-gnu -emit-llvm %s -o - | FileCheck %s + +#include "Inputs/stdio.h" + +int printf(const char *fmt, ...) { +return 0; +} + +void unit1() { + struct U1A { +short a; + }; + + struct U1A a = { + .a = 12, + }; + + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[RES1:%[0-9]+]] = getelementptr inbounds %struct.U1A, %struct.U1A* %a, i32 0, i32 0 + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[LOAD1:%[0-9]+]] = load i16, i16* [[RES1]], + // CHECK: call i32 (i8*, ...) @printf({{.*}}, i16 [[LOAD1]]) + // CHECK: call i32 (i8*, ...) @printf( + __builtin_dump_struct(, ); +} + +void unit2() { + struct U2A { +unsigned short a; + }; + + struct U2A a = { + .a = 12, + }; + + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[RES1:%[0-9]+]] = getelementptr inbounds %struct.U2A, %struct.U2A* %a, i32 0, i32 0 + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[LOAD1:%[0-9]+]] = load i16, i16* [[RES1]], + // CHECK: call i32 (i8*, ...) @printf({{.*}}, i16 [[LOAD1]]) + // CHECK: call i32 (i8*, ...) @printf( + __builtin_dump_struct(, ); +} + +void unit3() { + struct U3A { +int a; + }; + + struct U3A a = { + .a = 12, + }; + + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[RES1:%[0-9]+]] = getelementptr inbounds %struct.U3A, %struct.U3A* %a, i32
[PATCH] D44093: [BUILTINS] structure pretty printer
aaron.ballman closed this revision. aaron.ballman added a comment. I see we all found the solution at the same time. Yay teamwork! :-D I've committed (with the test fixed up) in r329762. Repository: rC Clang https://reviews.llvm.org/D44093 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44093: [BUILTINS] structure pretty printer
paulsemel added a comment. Thanks a lot for your help, updating the patch ! Repository: rC Clang https://reviews.llvm.org/D44093 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44093: [BUILTINS] structure pretty printer
arichardson added a comment. In https://reviews.llvm.org/D44093#1063679, @paulsemel wrote: > Ok, I found the problem. In fact the size of `long` is 4 bytes on your > machine, but 8 bytes on mine. > This makes this `// CHECK: [[LOAD1:%[0-9]+]] = load i64, i64* [[RES1]],` > fail. > Do you know a smart way to do it without dealing with type sizes ? You should probably run the test with `-triple=x86_64-unknown-linux` (or whatever the triple for your host machine happens to be). Otherwise they depend on the default clang triple which could have a different size long. Repository: rC Clang https://reviews.llvm.org/D44093 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44093: [BUILTINS] structure pretty printer
arichardson added a comment. In https://reviews.llvm.org/D44093#1063679, @paulsemel wrote: > Ok, I found the problem. In fact the size of `long` is 4 bytes on your > machine, but 8 bytes on mine. > This makes this `// CHECK: [[LOAD1:%[0-9]+]] = load i64, i64* [[RES1]],` > fail. > Do you know a smart way to do it without dealing with type sizes ? You should probably run the test with `-triple=x86_64-unknown-linux` (or whatever the triple for your host machine happens to be). Otherwise they depend on the default clang triple which could have a different size long. Repository: rC Clang https://reviews.llvm.org/D44093 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44093: [BUILTINS] structure pretty printer
lebedev.ri added inline comments. Comment at: test/CodeGen/dump-struct-builtin.c:1 +// RUN: %clang_cc1 -emit-llvm %s -o - | FileCheck %s + This should be ``` // RUN: %clang_cc1 -triple=x86_64-pc-linux-gnu -emit-llvm %s -o - | FileCheck %s ``` or something Repository: rC Clang https://reviews.llvm.org/D44093 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44093: [BUILTINS] structure pretty printer
paulsemel added a comment. Ok, I found the problem. In fact the size of `long` is 4 bytes on your machine, but 8 bytes on mine. This makes this `// CHECK: [[LOAD1:%[0-9]+]] = load i64, i64* [[RES1]],` fail. Do you know a smart way to do it without dealing with type sizes ? Repository: rC Clang https://reviews.llvm.org/D44093 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44093: [BUILTINS] structure pretty printer
paulsemel updated this revision to Diff 141913. paulsemel added a comment. Fixed printf warning generated in tests/CodeGen. Repository: rC Clang https://reviews.llvm.org/D44093 Files: include/clang/Basic/Builtins.def lib/CodeGen/CGBuiltin.cpp lib/Sema/SemaChecking.cpp test/CodeGen/dump-struct-builtin.c test/Sema/builtin-dump-struct.c Index: test/Sema/builtin-dump-struct.c === --- /dev/null +++ test/Sema/builtin-dump-struct.c @@ -0,0 +1,42 @@ +// RUN: %clang_cc1 -triple i386-unknown-unknown -fsyntax-only -fno-spell-checking -verify %s + +void invalid_uses() { + struct A { + }; + struct A a; + void *b; + int (*goodfunc)(const char *, ...); + int (*badfunc1)(const char *); + int (*badfunc2)(int, ...); + void (*badfunc3)(const char *, ...); + int (*badfunc4)(char *, ...); + int (*badfunc5)(void); + + __builtin_dump_struct(); // expected-error {{too few arguments to function call, expected 2, have 0}} + __builtin_dump_struct(1);// expected-error {{too few arguments to function call, expected 2, have 1}} + __builtin_dump_struct(1, 2); // expected-error {{passing 'int' to parameter of incompatible type structure pointer: type mismatch at 1st parameter ('int' vs structure pointer)}} + __builtin_dump_struct(, 2);// expected-error {{passing 'int' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int' vs 'int (*)(const char *, ...)')}} + __builtin_dump_struct(b, goodfunc); // expected-error {{passing 'void *' to parameter of incompatible type structure pointer: type mismatch at 1st parameter ('void *' vs structure pointer)}} + __builtin_dump_struct(, badfunc1); // expected-error {{passing 'int (*)(const char *)' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int (*)(const char *)' vs 'int (*)(const char *, ...)')}} + __builtin_dump_struct(, badfunc2); // expected-error {{passing 'int (*)(int, ...)' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int (*)(int, ...)' vs 'int (*)(const char *, ...)')}} + __builtin_dump_struct(, badfunc3); // expected-error {{passing 'void (*)(const char *, ...)' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('void (*)(const char *, ...)' vs 'int (*)(const char *, ...)')}} + __builtin_dump_struct(, badfunc4); // expected-error {{passing 'int (*)(char *, ...)' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int (*)(char *, ...)' vs 'int (*)(const char *, ...)')}} + __builtin_dump_struct(, badfunc5); // expected-error {{passing 'int (*)(void)' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int (*)(void)' vs 'int (*)(const char *, ...)')}} + __builtin_dump_struct(a, goodfunc); // expected-error {{passing 'struct A' to parameter of incompatible type structure pointer: type mismatch at 1st parameter ('struct A' vs structure pointer)}} +} + +void valid_uses() { + struct A { + }; + union B { + }; + + int (*goodfunc)(const char *, ...); + int (*goodfunc2)(); + struct A a; + union B b; + + __builtin_dump_struct(, goodfunc); + __builtin_dump_struct(, goodfunc); + __builtin_dump_struct(, goodfunc2); +} Index: test/CodeGen/dump-struct-builtin.c === --- /dev/null +++ test/CodeGen/dump-struct-builtin.c @@ -0,0 +1,391 @@ +// RUN: %clang_cc1 -emit-llvm %s -o - | FileCheck %s + +#include "Inputs/stdio.h" + +int printf(const char *fmt, ...) { +return 0; +} + +void unit1() { + struct U1A { +short a; + }; + + struct U1A a = { + .a = 12, + }; + + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[RES1:%[0-9]+]] = getelementptr inbounds %struct.U1A, %struct.U1A* %a, i32 0, i32 0 + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[LOAD1:%[0-9]+]] = load i16, i16* [[RES1]], + // CHECK: call i32 (i8*, ...) @printf({{.*}}, i16 [[LOAD1]]) + // CHECK: call i32 (i8*, ...) @printf( + __builtin_dump_struct(, ); +} + +void unit2() { + struct U2A { +unsigned short a; + }; + + struct U2A a = { + .a = 12, + }; + + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[RES1:%[0-9]+]] = getelementptr inbounds %struct.U2A, %struct.U2A* %a, i32 0, i32 0 + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[LOAD1:%[0-9]+]] = load i16, i16* [[RES1]], + // CHECK: call i32 (i8*, ...) @printf({{.*}}, i16 [[LOAD1]]) + // CHECK: call i32 (i8*, ...) @printf( + __builtin_dump_struct(, ); +} + +void unit3() { + struct U3A { +int a; + }; + + struct U3A a = { + .a = 12, + }; + + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[RES1:%[0-9]+]] = getelementptr inbounds %struct.U3A, %struct.U3A* %a, i32 0, i32 0 + // CHECK: call i32 (i8*, ...) @printf( + // CHECK:
[PATCH] D44093: [BUILTINS] structure pretty printer
paulsemel added a comment. Sorry about it, I also have the warning on my machine, but not the error you get... Those test are actually working on my different linux machines, that's really weird. This one is actually really weird, because I could find manually the missing pattern in your output.. I just don't get what is happening. Repository: rC Clang https://reviews.llvm.org/D44093 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44093: [BUILTINS] structure pretty printer
aaron.ballman added a comment. Thank you for rebasing, but the patch does not pass tests locally for me. I get the following results (testing on Windows x64 with MSVC 2017 in a Debug build): 63> TEST 'Clang :: CodeGen/dump-struct-builtin.c' FAILED 63>Script: 63>-- 63>d:\build\debug\bin\clang.EXE -cc1 -internal-isystem d:\build\debug\lib\clang\7.0.0\include -nostdsysteminc -emit-llvm D:\llvm\tools\clang\test\CodeGen\dump-struct-builtin.c -o - | D:\build\Debug\bin\FileCheck.EXE D:\llvm\tools\clang\test\CodeGen\dump-struct-builtin.c 63>-- 63>Exit Code: 1 63> 63>Command Output (stdout): 63>-- 63>$ "d:\build\debug\bin\clang.EXE" "-cc1" "-internal-isystem" "d:\build\debug\lib\clang\7.0.0\include" "-nostdsysteminc" "-emit-llvm" "D:\llvm\tools\clang\test\CodeGen\dump-struct-builtin.c" "-o" "-" 63># command stderr: 63>D:\llvm\tools\clang\test\CodeGen\dump-struct-builtin.c(20,30): warning G4AB12AC8: implicitly declaring library function 'printf' with type 'int (const char *, ...)' 63> 63> __builtin_dump_struct(, ); 63> 63> ^ 63> 63>D:\llvm\tools\clang\test\CodeGen\dump-struct-builtin.c:20:30: note: include the header or explicitly provide a declaration for 'printf' 63> 63>1 warning generated. 63> 63> 63>$ "D:\build\Debug\bin\FileCheck.EXE" "D:\llvm\tools\clang\test\CodeGen\dump-struct-builtin.c" 63># command stderr: 63>D:\llvm\tools\clang\test\CodeGen\dump-struct-builtin.c(105,12): error G8E235623: expected string not found in input 63> 63> // CHECK: [[RES1:%[0-9]+]] = getelementptr inbounds %struct.U6A, %struct.U6A* %a, i32 0, i32 0 63> 63> ^ 63> 63>:290:35: note: scanning from here 63> 63> %1 = call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([14 x i8], [14 x i8]* @28, i32 0, i32 0)) 63> 63> ^ 63> 63>:291:2: note: possible intended match here 63> 63> %2 = getelementptr inbounds %struct.U8A, %struct.U8A* %a, i32 0, i32 0 63> 63> ^ 63> 63> 63>CUSTOMBUILD : error : command failed with exit status: 1 63> 63>-- 63> 63> The output from the test on my machine is: C:\Program Files (x86)\Microsoft Visual Studio\2017\Professional>d:\build\debug\bin\clang.EXE -cc1 -internal-isystem d:\build\debug\lib\clang\7.0.0\include -nostdsysteminc -emit-llvm D:\llvm\tools\clang\test\CodeGen\dump-struct-builtin.c -o - D:\llvm\tools\clang\test\CodeGen\dump-struct-builtin.c:20:30: warning: implicitly declaring library function 'printf' with type 'int (const char *, ...)' __builtin_dump_struct(, ); ^ D:\llvm\tools\clang\test\CodeGen\dump-struct-builtin.c:20:30: note: include the header or explicitly provide a declaration for 'printf' ; ModuleID = 'D:\llvm\tools\clang\test\CodeGen\dump-struct-builtin.c' source_filename = "D:\5Cllvm\5Ctools\5Cclang\5Ctest\5CCodeGen\5Cdump-struct-builtin.c" target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128" target triple = "x86_64-pc-windows-msvc" %struct.U1A = type { i16 } %struct.U2A = type { i16 } %struct.U3A = type { i32 } %struct.U4A = type { i32 } %struct.U5A = type { i32 } %struct.U6A = type { i32 } %struct.U7A = type { i64 } %struct.U8A = type { i64 } %struct.U9A = type { i8 } %struct.U10A = type { i8* } %struct.U11A = type { i8* } %struct.U12A = type { i8* } %struct.U13A = type { i8* } %struct.U14A = type { double } %struct.U15A = type { [3 x i32] } %struct.T1A = type { i32, i8* } %struct.T2B = type { i32, %struct.T2A } %struct.T2A = type { i32 } %struct.T3A = type { %union.anon } %union.anon = type { i32 } %struct.T4A = type { %union.anon.0 } %union.anon.0 = type { %struct.anon } %struct.anon = type { i8* } %struct.anon.1 = type { i32 } $"??_C@_03OMINOMMP@LSE?$AA@" = comdat any @unit1.a = private unnamed_addr constant %struct.U1A { i16 12 }, align 2 @0 = private unnamed_addr constant [14 x i8] c"struct U1A {\0A\00" @1 = private unnamed_addr constant [11 x i8] c"short a : \00" @2 = private unnamed_addr constant [5 x i8] c"%hd\0A\00" @3 = private unnamed_addr constant [3 x i8] c"}\0A\00" @unit2.a = private unnamed_addr constant %struct.U2A { i16 12 }, align 2 @4 = private unnamed_addr constant [14 x i8] c"struct U2A {\0A\00" @5 = private unnamed_addr constant [20 x i8] c"unsigned short a : \00" @6 = private unnamed_addr constant [5 x i8] c"%hu\0A\00" @7 = private unnamed_addr constant [3 x i8] c"}\0A\00" @unit3.a = private unnamed_addr constant %struct.U3A { i32 12 }, align 4 @8 = private unnamed_addr constant [14 x i8] c"struct U3A {\0A\00" @9 = private unnamed_addr constant [9 x i8] c"int a : \00" @10 = private unnamed_addr constant [4 x i8] c"%d\0A\00" @11 = private unnamed_addr constant [3 x i8] c"}\0A\00" @unit4.a = private unnamed_addr constant %struct.U4A { i32 12 }, align 4 @12 = private unnamed_addr constant [14 x i8] c"struct U4A {\0A\00" @13 = private unnamed_addr constant [18 x i8] c"unsigned int a : \00" @14 = private unnamed_addr constant [4 x i8] c"%u\0A\00" @15 = private
[PATCH] D44093: [BUILTINS] structure pretty printer
paulsemel updated this revision to Diff 141890. paulsemel added a comment. Patch rebased. Minor fix for single quotes escaping. Repository: rC Clang https://reviews.llvm.org/D44093 Files: include/clang/Basic/Builtins.def lib/CodeGen/CGBuiltin.cpp lib/Sema/SemaChecking.cpp test/CodeGen/dump-struct-builtin.c test/Sema/builtin-dump-struct.c Index: test/Sema/builtin-dump-struct.c === --- /dev/null +++ test/Sema/builtin-dump-struct.c @@ -0,0 +1,42 @@ +// RUN: %clang_cc1 -triple i386-unknown-unknown -fsyntax-only -fno-spell-checking -verify %s + +void invalid_uses() { + struct A { + }; + struct A a; + void *b; + int (*goodfunc)(const char *, ...); + int (*badfunc1)(const char *); + int (*badfunc2)(int, ...); + void (*badfunc3)(const char *, ...); + int (*badfunc4)(char *, ...); + int (*badfunc5)(void); + + __builtin_dump_struct(); // expected-error {{too few arguments to function call, expected 2, have 0}} + __builtin_dump_struct(1);// expected-error {{too few arguments to function call, expected 2, have 1}} + __builtin_dump_struct(1, 2); // expected-error {{passing 'int' to parameter of incompatible type structure pointer: type mismatch at 1st parameter ('int' vs structure pointer)}} + __builtin_dump_struct(, 2);// expected-error {{passing 'int' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int' vs 'int (*)(const char *, ...)')}} + __builtin_dump_struct(b, goodfunc); // expected-error {{passing 'void *' to parameter of incompatible type structure pointer: type mismatch at 1st parameter ('void *' vs structure pointer)}} + __builtin_dump_struct(, badfunc1); // expected-error {{passing 'int (*)(const char *)' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int (*)(const char *)' vs 'int (*)(const char *, ...)')}} + __builtin_dump_struct(, badfunc2); // expected-error {{passing 'int (*)(int, ...)' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int (*)(int, ...)' vs 'int (*)(const char *, ...)')}} + __builtin_dump_struct(, badfunc3); // expected-error {{passing 'void (*)(const char *, ...)' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('void (*)(const char *, ...)' vs 'int (*)(const char *, ...)')}} + __builtin_dump_struct(, badfunc4); // expected-error {{passing 'int (*)(char *, ...)' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int (*)(char *, ...)' vs 'int (*)(const char *, ...)')}} + __builtin_dump_struct(, badfunc5); // expected-error {{passing 'int (*)(void)' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int (*)(void)' vs 'int (*)(const char *, ...)')}} + __builtin_dump_struct(a, goodfunc); // expected-error {{passing 'struct A' to parameter of incompatible type structure pointer: type mismatch at 1st parameter ('struct A' vs structure pointer)}} +} + +void valid_uses() { + struct A { + }; + union B { + }; + + int (*goodfunc)(const char *, ...); + int (*goodfunc2)(); + struct A a; + union B b; + + __builtin_dump_struct(, goodfunc); + __builtin_dump_struct(, goodfunc); + __builtin_dump_struct(, goodfunc2); +} Index: test/CodeGen/dump-struct-builtin.c === --- /dev/null +++ test/CodeGen/dump-struct-builtin.c @@ -0,0 +1,387 @@ +// RUN: %clang_cc1 -emit-llvm %s -o - | FileCheck %s + +#include "Inputs/stdio.h" + +void unit1() { + struct U1A { +short a; + }; + + struct U1A a = { + .a = 12, + }; + + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[RES1:%[0-9]+]] = getelementptr inbounds %struct.U1A, %struct.U1A* %a, i32 0, i32 0 + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[LOAD1:%[0-9]+]] = load i16, i16* [[RES1]], + // CHECK: call i32 (i8*, ...) @printf({{.*}}, i16 [[LOAD1]]) + // CHECK: call i32 (i8*, ...) @printf( + __builtin_dump_struct(, ); +} + +void unit2() { + struct U2A { +unsigned short a; + }; + + struct U2A a = { + .a = 12, + }; + + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[RES1:%[0-9]+]] = getelementptr inbounds %struct.U2A, %struct.U2A* %a, i32 0, i32 0 + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[LOAD1:%[0-9]+]] = load i16, i16* [[RES1]], + // CHECK: call i32 (i8*, ...) @printf({{.*}}, i16 [[LOAD1]]) + // CHECK: call i32 (i8*, ...) @printf( + __builtin_dump_struct(, ); +} + +void unit3() { + struct U3A { +int a; + }; + + struct U3A a = { + .a = 12, + }; + + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[RES1:%[0-9]+]] = getelementptr inbounds %struct.U3A, %struct.U3A* %a, i32 0, i32 0 + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[LOAD1:%[0-9]+]] = load i32, i32* [[RES1]], + // CHECK:
[PATCH] D44093: [BUILTINS] structure pretty printer
aaron.ballman added a comment. Can you rebase the patch? The patch did not apply cleanly for me against trunk (the contents of the check function are appearing in `CheckARMBuiltinExclusiveCall()`). (Also, one tiny nit about escaped characters that can be fixed at the same time.) Comment at: lib/Sema/SemaChecking.cpp:1133 + Diag(FnPtrArg->getLocStart(), diag::err_typecheck_convert_incompatible) + << FnPtrArgType << "\'int (*)(const char *, ...)\'" << 1 << 0 << 3 + << 2 << FnPtrArgType << "\'int (*)(const char *, ...)\'"; Minor nit: can you remove the escape character for the single quotes (here and elsewhere in the patch), as there's no need to escape that character? Repository: rC Clang https://reviews.llvm.org/D44093 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44093: [BUILTINS] structure pretty printer
paulsemel added a comment. In https://reviews.llvm.org/D44093#1063348, @aaron.ballman wrote: > In https://reviews.llvm.org/D44093#1063340, @paulsemel wrote: > > > I don't really know what's the procedure right now.. What should I do once > > you both accepted the patch ? :) > > > You're good to commit the patch and address the concerns raised by > @arichardson in a follow-up patch. Do you need someone to commit on your > behalf? Thanks! Yes please, I don't think I have the right accesses to do it myself Repository: rC Clang https://reviews.llvm.org/D44093 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44093: [BUILTINS] structure pretty printer
aaron.ballman added a comment. In https://reviews.llvm.org/D44093#1063340, @paulsemel wrote: > I don't really know what's the procedure right now.. What should I do once > you both accepted the patch ? :) You're good to commit the patch and address the concerns raised by @arichardson in a follow-up patch. Do you need someone to commit on your behalf? Repository: rC Clang https://reviews.llvm.org/D44093 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44093: [BUILTINS] structure pretty printer
paulsemel added a comment. I don't really know what's the procedure right now.. What should I do once you both accepted the patch ? :) Repository: rC Clang https://reviews.llvm.org/D44093 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44093: [BUILTINS] structure pretty printer
arichardson accepted this revision. arichardson added a comment. > So, for the moment, we are only handling basic types. That said, for the enum > in C, we will print according to the type of the enum. > In the future versions, I really want to be able to print the name of the > enum so that the output is more relevent. > Anyway, the rule I followed for the moment is : if I don't recognize the > type, I print it as an address. I you just print the address for unrecognized types then that should be fine. Thanks Comment at: test/Sema/builtin-dump-struct.c:42 + __builtin_dump_struct(, goodfunc2); +} paulsemel wrote: > arichardson wrote: > > I think there should also be a test here that we get an error when the > > struct contains bitfields instead of crashing/generating nonsense in > > CodeGen. > Do you really think that I should throw an error just because there is a > bitfield ? > I was thinking about just accepting the fact that the bitfield outputs are > not correct but permit the user to pretty print the remaining part of the > structure. > What do you think ? I would prefer an error instead of incorrect output since that can result in lots of unncessary debug work/false conclusions. But as long as it gets fixed/turned into an error before the final 7.0 release I think it shouldn't matter. Repository: rC Clang https://reviews.llvm.org/D44093 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44093: [BUILTINS] structure pretty printer
paulsemel added a comment. In https://reviews.llvm.org/D44093#1063122, @arichardson wrote: > I'm also often restricted to using printf for debugging so this looks really > useful! > > However, before committing this I feel like the test should also verify that > the format strings that are generated are sensible. > > Also what should happens when you have enum members in your struct or maybe > even C++ pointers to members? So, for the moment, we are only handling basic types. That said, for the enum in C, we will print according to the type of the enum. In the future versions, I really want to be able to print the name of the enum so that the output is more relevent. Anyway, the rule I followed for the moment is : if I don't recognize the type, I print it as an address. Comment at: test/Sema/builtin-dump-struct.c:42 + __builtin_dump_struct(, goodfunc2); +} arichardson wrote: > I think there should also be a test here that we get an error when the struct > contains bitfields instead of crashing/generating nonsense in CodeGen. Do you really think that I should throw an error just because there is a bitfield ? I was thinking about just accepting the fact that the bitfield outputs are not correct but permit the user to pretty print the remaining part of the structure. What do you think ? Repository: rC Clang https://reviews.llvm.org/D44093 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44093: [BUILTINS] structure pretty printer
arichardson added a comment. I'm also often restricted to using printf for debugging so this looks really useful! However, before committing this I feel like the test should also verify that the format strings that are generated are sensible. Also what should happens when you have enum members in your struct or maybe even C++ pointers to members? Comment at: test/Sema/builtin-dump-struct.c:42 + __builtin_dump_struct(, goodfunc2); +} I think there should also be a test here that we get an error when the struct contains bitfields instead of crashing/generating nonsense in CodeGen. Repository: rC Clang https://reviews.llvm.org/D44093 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44093: [BUILTINS] structure pretty printer
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM with some minor commenting nits. Comment at: lib/CodeGen/CGBuiltin.cpp:992 + +// We check whether we are in a recursive type +if (CanonicalType->isRecordType()) { Missing full stop at the end of the comment. Same elsewhere. Comment at: lib/Sema/SemaChecking.cpp:1114 + case Builtin::BI__builtin_dump_struct: { +// We first want to ensure we are called with 2 arguments +if (checkArgCount(*this, TheCall, 2)) Missing full stop here and elsewhere as well. Repository: rC Clang https://reviews.llvm.org/D44093 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44093: [BUILTINS] structure pretty printer
paulsemel updated this revision to Diff 141762. paulsemel added a comment. Added a good test for the `int (*)()` function prototype, as we decided to accept it as a valid function for the dumper Repository: rC Clang https://reviews.llvm.org/D44093 Files: include/clang/Basic/Builtins.def lib/CodeGen/CGBuiltin.cpp lib/Sema/SemaChecking.cpp test/CodeGen/dump-struct-builtin.c test/Sema/builtin-dump-struct.c Index: test/Sema/builtin-dump-struct.c === --- /dev/null +++ test/Sema/builtin-dump-struct.c @@ -0,0 +1,42 @@ +// RUN: %clang_cc1 -triple i386-unknown-unknown -fsyntax-only -fno-spell-checking -verify %s + +void invalid_uses() { + struct A { + }; + struct A a; + void *b; + int (*goodfunc)(const char *, ...); + int (*badfunc1)(const char *); + int (*badfunc2)(int, ...); + void (*badfunc3)(const char *, ...); + int (*badfunc4)(char *, ...); + int (*badfunc5)(void); + + __builtin_dump_struct(); // expected-error {{too few arguments to function call, expected 2, have 0}} + __builtin_dump_struct(1);// expected-error {{too few arguments to function call, expected 2, have 1}} + __builtin_dump_struct(1, 2); // expected-error {{passing 'int' to parameter of incompatible type structure pointer: type mismatch at 1st parameter ('int' vs structure pointer)}} + __builtin_dump_struct(, 2);// expected-error {{passing 'int' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int' vs 'int (*)(const char *, ...)')}} + __builtin_dump_struct(b, goodfunc); // expected-error {{passing 'void *' to parameter of incompatible type structure pointer: type mismatch at 1st parameter ('void *' vs structure pointer)}} + __builtin_dump_struct(, badfunc1); // expected-error {{passing 'int (*)(const char *)' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int (*)(const char *)' vs 'int (*)(const char *, ...)')}} + __builtin_dump_struct(, badfunc2); // expected-error {{passing 'int (*)(int, ...)' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int (*)(int, ...)' vs 'int (*)(const char *, ...)')}} + __builtin_dump_struct(, badfunc3); // expected-error {{passing 'void (*)(const char *, ...)' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('void (*)(const char *, ...)' vs 'int (*)(const char *, ...)')}} + __builtin_dump_struct(, badfunc4); // expected-error {{passing 'int (*)(char *, ...)' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int (*)(char *, ...)' vs 'int (*)(const char *, ...)')}} + __builtin_dump_struct(, badfunc5); // expected-error {{passing 'int (*)(void)' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int (*)(void)' vs 'int (*)(const char *, ...)')}} + __builtin_dump_struct(a, goodfunc); // expected-error {{passing 'struct A' to parameter of incompatible type structure pointer: type mismatch at 1st parameter ('struct A' vs structure pointer)}} +} + +void valid_uses() { + struct A { + }; + union B { + }; + + int (*goodfunc)(const char *, ...); + int (*goodfunc2)(); + struct A a; + union B b; + + __builtin_dump_struct(, goodfunc); + __builtin_dump_struct(, goodfunc); + __builtin_dump_struct(, goodfunc2); +} Index: test/CodeGen/dump-struct-builtin.c === --- /dev/null +++ test/CodeGen/dump-struct-builtin.c @@ -0,0 +1,387 @@ +// RUN: %clang_cc1 -emit-llvm %s -o - | FileCheck %s + +#include "Inputs/stdio.h" + +void unit1() { + struct U1A { +short a; + }; + + struct U1A a = { + .a = 12, + }; + + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[RES1:%[0-9]+]] = getelementptr inbounds %struct.U1A, %struct.U1A* %a, i32 0, i32 0 + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[LOAD1:%[0-9]+]] = load i16, i16* [[RES1]], + // CHECK: call i32 (i8*, ...) @printf({{.*}}, i16 [[LOAD1]]) + // CHECK: call i32 (i8*, ...) @printf( + __builtin_dump_struct(, ); +} + +void unit2() { + struct U2A { +unsigned short a; + }; + + struct U2A a = { + .a = 12, + }; + + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[RES1:%[0-9]+]] = getelementptr inbounds %struct.U2A, %struct.U2A* %a, i32 0, i32 0 + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[LOAD1:%[0-9]+]] = load i16, i16* [[RES1]], + // CHECK: call i32 (i8*, ...) @printf({{.*}}, i16 [[LOAD1]]) + // CHECK: call i32 (i8*, ...) @printf( + __builtin_dump_struct(, ); +} + +void unit3() { + struct U3A { +int a; + }; + + struct U3A a = { + .a = 12, + }; + + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[RES1:%[0-9]+]] = getelementptr inbounds %struct.U3A, %struct.U3A* %a, i32 0, i32 0 + // CHECK: call i32 (i8*, ...) @printf( +
[PATCH] D44093: [BUILTINS] structure pretty printer
paulsemel added inline comments. Comment at: test/Sema/builtin-dump-struct.c:8 + void *b; + int (*goodfunc)(const char *, ...); + int (*badfunc1)(const char *); aaron.ballman wrote: > paulsemel wrote: > > aaron.ballman wrote: > > > paulsemel wrote: > > > > aaron.ballman wrote: > > > > > Can you also add a test for: `int (*badfunc4)(char *, ...);` and `int > > > > > (*badfunc5)();` > > > > Isn't `int (*func)()` is a valid prototype for a printf like function > > > > in C ? > > > > I instead added `int (*func)(void)` to the test cases. > > > > Isn't int (*func)() is a valid prototype for a printf like function in > > > > C ? > > > > > > No, because it's missing the `const char *` as the mandatory first > > > parameter. Do you want that to be allowed and hope the callee has it > > > correct on their side, or do you want it to diagnose as not being a valid > > > function? > > Actually, from a kernel developer perspective, I would say it's better to > > let the user do its stuff on his side, because kernel is full of trick ! > > But if you think I'd rather check whether we have `int (*)(const char *, > > ...)` at any time, we can go for it ! > Okay, if you think it'd be beneficial to allow a function without a > prototype, I'm okay with it. Can you make it an explicit "good" test case? Sure :) Repository: rC Clang https://reviews.llvm.org/D44093 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44093: [BUILTINS] structure pretty printer
aaron.ballman added inline comments. Comment at: test/Sema/builtin-dump-struct.c:8 + void *b; + int (*goodfunc)(const char *, ...); + int (*badfunc1)(const char *); paulsemel wrote: > aaron.ballman wrote: > > paulsemel wrote: > > > aaron.ballman wrote: > > > > Can you also add a test for: `int (*badfunc4)(char *, ...);` and `int > > > > (*badfunc5)();` > > > Isn't `int (*func)()` is a valid prototype for a printf like function in > > > C ? > > > I instead added `int (*func)(void)` to the test cases. > > > Isn't int (*func)() is a valid prototype for a printf like function in C ? > > > > No, because it's missing the `const char *` as the mandatory first > > parameter. Do you want that to be allowed and hope the callee has it > > correct on their side, or do you want it to diagnose as not being a valid > > function? > Actually, from a kernel developer perspective, I would say it's better to let > the user do its stuff on his side, because kernel is full of trick ! > But if you think I'd rather check whether we have `int (*)(const char *, > ...)` at any time, we can go for it ! Okay, if you think it'd be beneficial to allow a function without a prototype, I'm okay with it. Can you make it an explicit "good" test case? Repository: rC Clang https://reviews.llvm.org/D44093 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44093: [BUILTINS] structure pretty printer
paulsemel updated this revision to Diff 141745. paulsemel marked an inline comment as done. paulsemel added a comment. Updated the amperstamp in the Sema test to be consistent with the remaining part of it. Repository: rC Clang https://reviews.llvm.org/D44093 Files: include/clang/Basic/Builtins.def lib/CodeGen/CGBuiltin.cpp lib/Sema/SemaChecking.cpp test/CodeGen/dump-struct-builtin.c test/Sema/builtin-dump-struct.c Index: test/Sema/builtin-dump-struct.c === --- /dev/null +++ test/Sema/builtin-dump-struct.c @@ -0,0 +1,40 @@ +// RUN: %clang_cc1 -triple i386-unknown-unknown -fsyntax-only -fno-spell-checking -verify %s + +void invalid_uses() { + struct A { + }; + struct A a; + void *b; + int (*goodfunc)(const char *, ...); + int (*badfunc1)(const char *); + int (*badfunc2)(int, ...); + void (*badfunc3)(const char *, ...); + int (*badfunc4)(char *, ...); + int (*badfunc5)(void); + + __builtin_dump_struct(); // expected-error {{too few arguments to function call, expected 2, have 0}} + __builtin_dump_struct(1);// expected-error {{too few arguments to function call, expected 2, have 1}} + __builtin_dump_struct(1, 2); // expected-error {{passing 'int' to parameter of incompatible type structure pointer: type mismatch at 1st parameter ('int' vs structure pointer)}} + __builtin_dump_struct(, 2);// expected-error {{passing 'int' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int' vs 'int (*)(const char *, ...)')}} + __builtin_dump_struct(b, goodfunc); // expected-error {{passing 'void *' to parameter of incompatible type structure pointer: type mismatch at 1st parameter ('void *' vs structure pointer)}} + __builtin_dump_struct(, badfunc1); // expected-error {{passing 'int (*)(const char *)' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int (*)(const char *)' vs 'int (*)(const char *, ...)')}} + __builtin_dump_struct(, badfunc2); // expected-error {{passing 'int (*)(int, ...)' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int (*)(int, ...)' vs 'int (*)(const char *, ...)')}} + __builtin_dump_struct(, badfunc3); // expected-error {{passing 'void (*)(const char *, ...)' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('void (*)(const char *, ...)' vs 'int (*)(const char *, ...)')}} + __builtin_dump_struct(, badfunc4); // expected-error {{passing 'int (*)(char *, ...)' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int (*)(char *, ...)' vs 'int (*)(const char *, ...)')}} + __builtin_dump_struct(, badfunc5); // expected-error {{passing 'int (*)(void)' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int (*)(void)' vs 'int (*)(const char *, ...)')}} + __builtin_dump_struct(a, goodfunc); // expected-error {{passing 'struct A' to parameter of incompatible type structure pointer: type mismatch at 1st parameter ('struct A' vs structure pointer)}} +} + +void valid_uses() { + struct A { + }; + union B { + }; + + int (*goodfunc)(const char *, ...); + struct A a; + union B b; + + __builtin_dump_struct(, goodfunc); + __builtin_dump_struct(, goodfunc); +} Index: test/CodeGen/dump-struct-builtin.c === --- /dev/null +++ test/CodeGen/dump-struct-builtin.c @@ -0,0 +1,387 @@ +// RUN: %clang_cc1 -emit-llvm %s -o - | FileCheck %s + +#include "Inputs/stdio.h" + +void unit1() { + struct U1A { +short a; + }; + + struct U1A a = { + .a = 12, + }; + + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[RES1:%[0-9]+]] = getelementptr inbounds %struct.U1A, %struct.U1A* %a, i32 0, i32 0 + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[LOAD1:%[0-9]+]] = load i16, i16* [[RES1]], + // CHECK: call i32 (i8*, ...) @printf({{.*}}, i16 [[LOAD1]]) + // CHECK: call i32 (i8*, ...) @printf( + __builtin_dump_struct(, ); +} + +void unit2() { + struct U2A { +unsigned short a; + }; + + struct U2A a = { + .a = 12, + }; + + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[RES1:%[0-9]+]] = getelementptr inbounds %struct.U2A, %struct.U2A* %a, i32 0, i32 0 + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[LOAD1:%[0-9]+]] = load i16, i16* [[RES1]], + // CHECK: call i32 (i8*, ...) @printf({{.*}}, i16 [[LOAD1]]) + // CHECK: call i32 (i8*, ...) @printf( + __builtin_dump_struct(, ); +} + +void unit3() { + struct U3A { +int a; + }; + + struct U3A a = { + .a = 12, + }; + + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[RES1:%[0-9]+]] = getelementptr inbounds %struct.U3A, %struct.U3A* %a, i32 0, i32 0 + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[LOAD1:%[0-9]+]] = load i32, i32*
[PATCH] D44093: [BUILTINS] structure pretty printer
paulsemel marked an inline comment as done. paulsemel added a comment. No problem, thanks for getting back on this ! I was busy because of my midterms anyway :) Comment at: test/Sema/builtin-dump-struct.c:8 + void *b; + int (*goodfunc)(const char *, ...); + int (*badfunc1)(const char *); aaron.ballman wrote: > paulsemel wrote: > > aaron.ballman wrote: > > > Can you also add a test for: `int (*badfunc4)(char *, ...);` and `int > > > (*badfunc5)();` > > Isn't `int (*func)()` is a valid prototype for a printf like function in C ? > > I instead added `int (*func)(void)` to the test cases. > > Isn't int (*func)() is a valid prototype for a printf like function in C ? > > No, because it's missing the `const char *` as the mandatory first parameter. > Do you want that to be allowed and hope the callee has it correct on their > side, or do you want it to diagnose as not being a valid function? Actually, from a kernel developer perspective, I would say it's better to let the user do its stuff on his side, because kernel is full of trick ! But if you think I'd rather check whether we have `int (*)(const char *, ...)` at any time, we can go for it ! Comment at: test/Sema/builtin-dump-struct.c:17 + __builtin_dump_struct(, 2);// expected-error {{passing 'int' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int' vs 'int (*)(const char *, ...)')}} + __builtin_dump_struct(b, ); // expected-error {{passing 'void *' to parameter of incompatible type 'structure pointer type': type mismatch at 1st parameter ('void *' vs 'structure pointer type')}} + __builtin_dump_struct(, badfunc1); // expected-error {{passing 'int (*)(const char *)' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int (*)(const char *)' vs 'int (*)(const char *, ...)')}} aaron.ballman wrote: > paulsemel wrote: > > aaron.ballman wrote: > > > Why ``? > > Yes, we already have a test like this anyway :) > It was more a question of why the ampersand on the second argument -- I think > that can be dropped and it'll be consistent with the rest of the tests. Sure, sorry, I missed this. Totally agree with you, I am going to make the changes. Repository: rC Clang https://reviews.llvm.org/D44093 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44093: [BUILTINS] structure pretty printer
aaron.ballman added a comment. Sorry for the delayed review; I'm back from vacation now and am picking things up again. Comment at: test/Sema/builtin-dump-struct.c:8 + void *b; + int (*goodfunc)(const char *, ...); + int (*badfunc1)(const char *); paulsemel wrote: > aaron.ballman wrote: > > Can you also add a test for: `int (*badfunc4)(char *, ...);` and `int > > (*badfunc5)();` > Isn't `int (*func)()` is a valid prototype for a printf like function in C ? > I instead added `int (*func)(void)` to the test cases. > Isn't int (*func)() is a valid prototype for a printf like function in C ? No, because it's missing the `const char *` as the mandatory first parameter. Do you want that to be allowed and hope the callee has it correct on their side, or do you want it to diagnose as not being a valid function? Comment at: test/Sema/builtin-dump-struct.c:15 + __builtin_dump_struct(1);// expected-error {{too few arguments to function call, expected 2, have 1}} + __builtin_dump_struct(1, 2); // expected-error {{passing 'int' to parameter of incompatible type 'structure pointer type': type mismatch at 1st parameter ('int' vs 'structure pointer type')}} + __builtin_dump_struct(, 2);// expected-error {{passing 'int' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int' vs 'int (*)(const char *, ...)')}} paulsemel wrote: > aaron.ballman wrote: > > Hrm, the `'structure pointer type'` in this diagnostic is unfortunate > > because it's being quoted as though it were a real type -- you could drop > > the single quotes. If you think the resulting diagnostic reads too > > strangely, perhaps we will have to go back to a custom diagnostic after all. > I think it will be better to just put something like `structure pointer`, so > that we understand the type we are talking about. > But this diagnostic seems great, still, what do you think about sticking with > it ? I think it's fine now; if it turns out to be horribly confusing to users, we can address it later. Comment at: test/Sema/builtin-dump-struct.c:17 + __builtin_dump_struct(, 2);// expected-error {{passing 'int' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int' vs 'int (*)(const char *, ...)')}} + __builtin_dump_struct(b, ); // expected-error {{passing 'void *' to parameter of incompatible type 'structure pointer type': type mismatch at 1st parameter ('void *' vs 'structure pointer type')}} + __builtin_dump_struct(, badfunc1); // expected-error {{passing 'int (*)(const char *)' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int (*)(const char *)' vs 'int (*)(const char *, ...)')}} paulsemel wrote: > aaron.ballman wrote: > > Why ``? > Yes, we already have a test like this anyway :) It was more a question of why the ampersand on the second argument -- I think that can be dropped and it'll be consistent with the rest of the tests. Repository: rC Clang https://reviews.llvm.org/D44093 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44093: [BUILTINS] structure pretty printer
paulsemel updated this revision to Diff 140304. paulsemel marked 3 inline comments as done. paulsemel added a comment. Added Aaron suggestions. Fixed a bug when having nested anonymous unions and structures. Repository: rC Clang https://reviews.llvm.org/D44093 Files: include/clang/Basic/Builtins.def lib/CodeGen/CGBuiltin.cpp lib/Sema/SemaChecking.cpp test/CodeGen/dump-struct-builtin.c test/Sema/builtin-dump-struct.c Index: test/Sema/builtin-dump-struct.c === --- /dev/null +++ test/Sema/builtin-dump-struct.c @@ -0,0 +1,40 @@ +// RUN: %clang_cc1 -triple i386-unknown-unknown -fsyntax-only -fno-spell-checking -verify %s + +void invalid_uses() { + struct A { + }; + struct A a; + void *b; + int (*goodfunc)(const char *, ...); + int (*badfunc1)(const char *); + int (*badfunc2)(int, ...); + void (*badfunc3)(const char *, ...); + int (*badfunc4)(char *, ...); + int (*badfunc5)(void); + + __builtin_dump_struct(); // expected-error {{too few arguments to function call, expected 2, have 0}} + __builtin_dump_struct(1);// expected-error {{too few arguments to function call, expected 2, have 1}} + __builtin_dump_struct(1, 2); // expected-error {{passing 'int' to parameter of incompatible type structure pointer: type mismatch at 1st parameter ('int' vs structure pointer)}} + __builtin_dump_struct(, 2);// expected-error {{passing 'int' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int' vs 'int (*)(const char *, ...)')}} + __builtin_dump_struct(b, ); // expected-error {{passing 'void *' to parameter of incompatible type structure pointer: type mismatch at 1st parameter ('void *' vs structure pointer)}} + __builtin_dump_struct(, badfunc1); // expected-error {{passing 'int (*)(const char *)' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int (*)(const char *)' vs 'int (*)(const char *, ...)')}} + __builtin_dump_struct(, badfunc2); // expected-error {{passing 'int (*)(int, ...)' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int (*)(int, ...)' vs 'int (*)(const char *, ...)')}} + __builtin_dump_struct(, badfunc3); // expected-error {{passing 'void (*)(const char *, ...)' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('void (*)(const char *, ...)' vs 'int (*)(const char *, ...)')}} + __builtin_dump_struct(, badfunc4); // expected-error {{passing 'int (*)(char *, ...)' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int (*)(char *, ...)' vs 'int (*)(const char *, ...)')}} + __builtin_dump_struct(, badfunc5); // expected-error {{passing 'int (*)(void)' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int (*)(void)' vs 'int (*)(const char *, ...)')}} + __builtin_dump_struct(a, goodfunc); // expected-error {{passing 'struct A' to parameter of incompatible type structure pointer: type mismatch at 1st parameter ('struct A' vs structure pointer)}} +} + +void valid_uses() { + struct A { + }; + union B { + }; + + int (*goodfunc)(const char *, ...); + struct A a; + union B b; + + __builtin_dump_struct(, goodfunc); + __builtin_dump_struct(, goodfunc); +} Index: test/CodeGen/dump-struct-builtin.c === --- /dev/null +++ test/CodeGen/dump-struct-builtin.c @@ -0,0 +1,387 @@ +// RUN: %clang_cc1 -emit-llvm %s -o - | FileCheck %s + +#include "Inputs/stdio.h" + +void unit1() { + struct U1A { +short a; + }; + + struct U1A a = { + .a = 12, + }; + + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[RES1:%[0-9]+]] = getelementptr inbounds %struct.U1A, %struct.U1A* %a, i32 0, i32 0 + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[LOAD1:%[0-9]+]] = load i16, i16* [[RES1]], + // CHECK: call i32 (i8*, ...) @printf({{.*}}, i16 [[LOAD1]]) + // CHECK: call i32 (i8*, ...) @printf( + __builtin_dump_struct(, ); +} + +void unit2() { + struct U2A { +unsigned short a; + }; + + struct U2A a = { + .a = 12, + }; + + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[RES1:%[0-9]+]] = getelementptr inbounds %struct.U2A, %struct.U2A* %a, i32 0, i32 0 + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[LOAD1:%[0-9]+]] = load i16, i16* [[RES1]], + // CHECK: call i32 (i8*, ...) @printf({{.*}}, i16 [[LOAD1]]) + // CHECK: call i32 (i8*, ...) @printf( + __builtin_dump_struct(, ); +} + +void unit3() { + struct U3A { +int a; + }; + + struct U3A a = { + .a = 12, + }; + + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[RES1:%[0-9]+]] = getelementptr inbounds %struct.U3A, %struct.U3A* %a, i32 0, i32 0 + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[LOAD1:%[0-9]+]] = load i32, i32* [[RES1]], +
[PATCH] D44093: [BUILTINS] structure pretty printer
paulsemel marked 6 inline comments as done. paulsemel added inline comments. Comment at: test/Sema/builtin-dump-struct.c:8 + void *b; + int (*goodfunc)(const char *, ...); + int (*badfunc1)(const char *); aaron.ballman wrote: > Can you also add a test for: `int (*badfunc4)(char *, ...);` and `int > (*badfunc5)();` Isn't `int (*func)()` is a valid prototype for a printf like function in C ? I instead added `int (*func)(void)` to the test cases. Comment at: test/Sema/builtin-dump-struct.c:15 + __builtin_dump_struct(1);// expected-error {{too few arguments to function call, expected 2, have 1}} + __builtin_dump_struct(1, 2); // expected-error {{passing 'int' to parameter of incompatible type 'structure pointer type': type mismatch at 1st parameter ('int' vs 'structure pointer type')}} + __builtin_dump_struct(, 2);// expected-error {{passing 'int' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int' vs 'int (*)(const char *, ...)')}} aaron.ballman wrote: > Hrm, the `'structure pointer type'` in this diagnostic is unfortunate because > it's being quoted as though it were a real type -- you could drop the single > quotes. If you think the resulting diagnostic reads too strangely, perhaps we > will have to go back to a custom diagnostic after all. I think it will be better to just put something like `structure pointer`, so that we understand the type we are talking about. But this diagnostic seems great, still, what do you think about sticking with it ? Comment at: test/Sema/builtin-dump-struct.c:17 + __builtin_dump_struct(, 2);// expected-error {{passing 'int' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int' vs 'int (*)(const char *, ...)')}} + __builtin_dump_struct(b, ); // expected-error {{passing 'void *' to parameter of incompatible type 'structure pointer type': type mismatch at 1st parameter ('void *' vs 'structure pointer type')}} + __builtin_dump_struct(, badfunc1); // expected-error {{passing 'int (*)(const char *)' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int (*)(const char *)' vs 'int (*)(const char *, ...)')}} aaron.ballman wrote: > Why ``? Yes, we already have a test like this anyway :) Repository: rC Clang https://reviews.llvm.org/D44093 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44093: [BUILTINS] structure pretty printer
aaron.ballman added inline comments. Comment at: lib/Sema/SemaChecking.cpp:1122 +!PtrArgType->getPointeeType()->isRecordType()) { + this->Diag(PtrArg->getLocStart(), + diag::err_typecheck_convert_incompatible) Drop all instances of `this->`. Comment at: lib/Sema/SemaChecking.cpp:1156 + if (!FT->isVariadic() || FT->getReturnType() != Context.IntTy || + FT->getParamType(0) != firstArg) { +this->Diag(FnPtrArg->getLocStart(), Rather than building up a type only for comparison purposes, why not check: ``` QualType PT = FT->getParamType(0); if (!PT->isPointerType() || !PT->getPointeeType()->isCharType() || !PT->getPointeeType().isConstQualified()) { Diag(...); } ``` Comment at: test/CodeGen/dump-struct-builtin.c:254 + __builtin_dump_struct(, ); +} I'd like to see some test cases using unions (perhaps mixing unions and inner struct types), tests using arrays, floating-point values, and type qualifiers on the data members. Comment at: test/Sema/builtin-dump-struct.c:8 + void *b; + int (*goodfunc)(const char *, ...); + int (*badfunc1)(const char *); Can you also add a test for: `int (*badfunc4)(char *, ...);` and `int (*badfunc5)();` Comment at: test/Sema/builtin-dump-struct.c:15 + __builtin_dump_struct(1);// expected-error {{too few arguments to function call, expected 2, have 1}} + __builtin_dump_struct(1, 2); // expected-error {{passing 'int' to parameter of incompatible type 'structure pointer type': type mismatch at 1st parameter ('int' vs 'structure pointer type')}} + __builtin_dump_struct(, 2);// expected-error {{passing 'int' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int' vs 'int (*)(const char *, ...)')}} Hrm, the `'structure pointer type'` in this diagnostic is unfortunate because it's being quoted as though it were a real type -- you could drop the single quotes. If you think the resulting diagnostic reads too strangely, perhaps we will have to go back to a custom diagnostic after all. Comment at: test/Sema/builtin-dump-struct.c:17 + __builtin_dump_struct(, 2);// expected-error {{passing 'int' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int' vs 'int (*)(const char *, ...)')}} + __builtin_dump_struct(b, ); // expected-error {{passing 'void *' to parameter of incompatible type 'structure pointer type': type mismatch at 1st parameter ('void *' vs 'structure pointer type')}} + __builtin_dump_struct(, badfunc1); // expected-error {{passing 'int (*)(const char *)' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int (*)(const char *)' vs 'int (*)(const char *, ...)')}} Why ``? Repository: rC Clang https://reviews.llvm.org/D44093 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44093: [BUILTINS] structure pretty printer
paulsemel updated this revision to Diff 138832. paulsemel added a comment. Applied Aaron's suggestions Added Sema tests for the typechecking Repository: rC Clang https://reviews.llvm.org/D44093 Files: include/clang/Basic/Builtins.def lib/CodeGen/CGBuiltin.cpp lib/Sema/SemaChecking.cpp test/CodeGen/dump-struct-builtin.c test/Sema/builtin-dump-struct.c Index: test/Sema/builtin-dump-struct.c === --- /dev/null +++ test/Sema/builtin-dump-struct.c @@ -0,0 +1,36 @@ +// RUN: %clang_cc1 -triple i386-unknown-unknown -fsyntax-only -fno-spell-checking -verify %s + +void invalid_uses() { + struct A { + }; + struct A a; + void *b; + int (*goodfunc)(const char *, ...); + int (*badfunc1)(const char *); + int (*badfunc2)(int, ...); + void (*badfunc3)(const char *, ...); + + __builtin_dump_struct(); // expected-error {{too few arguments to function call, expected 2, have 0}} + __builtin_dump_struct(1);// expected-error {{too few arguments to function call, expected 2, have 1}} + __builtin_dump_struct(1, 2); // expected-error {{passing 'int' to parameter of incompatible type 'structure pointer type': type mismatch at 1st parameter ('int' vs 'structure pointer type')}} + __builtin_dump_struct(, 2);// expected-error {{passing 'int' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int' vs 'int (*)(const char *, ...)')}} + __builtin_dump_struct(b, ); // expected-error {{passing 'void *' to parameter of incompatible type 'structure pointer type': type mismatch at 1st parameter ('void *' vs 'structure pointer type')}} + __builtin_dump_struct(, badfunc1); // expected-error {{passing 'int (*)(const char *)' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int (*)(const char *)' vs 'int (*)(const char *, ...)')}} + __builtin_dump_struct(, badfunc2); // expected-error {{passing 'int (*)(int, ...)' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int (*)(int, ...)' vs 'int (*)(const char *, ...)')}} + __builtin_dump_struct(, badfunc3); // expected-error {{passing 'void (*)(const char *, ...)' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('void (*)(const char *, ...)' vs 'int (*)(const char *, ...)')}} + __builtin_dump_struct(a, goodfunc); // expected-error {{passing 'struct A' to parameter of incompatible type 'structure pointer type': type mismatch at 1st parameter ('struct A' vs 'structure pointer type')}} +} + +void valid_uses() { + struct A { + }; + union B { + }; + + int (*goodfunc)(const char *, ...); + struct A a; + union B b; + + __builtin_dump_struct(, goodfunc); + __builtin_dump_struct(, goodfunc); +} Index: test/CodeGen/dump-struct-builtin.c === --- /dev/null +++ test/CodeGen/dump-struct-builtin.c @@ -0,0 +1,254 @@ +// RUN: %clang_cc1 -emit-llvm %s -o - | FileCheck %s + +#include "Inputs/stdio.h" + +void unit1() { + struct U1A { +short a; + }; + + struct U1A a = { + .a = 12, + }; + + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[RES1:%[0-9]+]] = getelementptr inbounds %struct.U1A, %struct.U1A* %a, i32 0, i32 0 + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[LOAD1:%[0-9]+]] = load i16, i16* [[RES1]], + // CHECK: call i32 (i8*, ...) @printf({{.*}}, i16 [[LOAD1]]) + // CHECK: call i32 (i8*, ...) @printf( + __builtin_dump_struct(, ); +} + +void unit2() { + struct U2A { +unsigned short a; + }; + + struct U2A a = { + .a = 12, + }; + + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[RES1:%[0-9]+]] = getelementptr inbounds %struct.U2A, %struct.U2A* %a, i32 0, i32 0 + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[LOAD1:%[0-9]+]] = load i16, i16* [[RES1]], + // CHECK: call i32 (i8*, ...) @printf({{.*}}, i16 [[LOAD1]]) + // CHECK: call i32 (i8*, ...) @printf( + __builtin_dump_struct(, ); +} + +void unit3() { + struct U3A { +int a; + }; + + struct U3A a = { + .a = 12, + }; + + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[RES1:%[0-9]+]] = getelementptr inbounds %struct.U3A, %struct.U3A* %a, i32 0, i32 0 + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[LOAD1:%[0-9]+]] = load i32, i32* [[RES1]], + // CHECK: call i32 (i8*, ...) @printf({{.*}}, i32 [[LOAD1]]) + // CHECK: call i32 (i8*, ...) @printf( + __builtin_dump_struct(, ); +} + +void unit4() { + struct U4A { +unsigned int a; + }; + + struct U4A a = { + .a = 12, + }; + + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[RES1:%[0-9]+]] = getelementptr inbounds %struct.U4A, %struct.U4A* %a, i32 0, i32 0 + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[LOAD1:%[0-9]+]] = load i32, i32* [[RES1]], + // CHECK: call i32 (i8*, ...) @printf({{.*}}, i32 [[LOAD1]]) + //
[PATCH] D44093: [BUILTINS] structure pretty printer
aaron.ballman added inline comments. Comment at: lib/CodeGen/CGBuiltin.cpp:935 +static llvm::Value *dumpRecord(CodeGenFunction , QualType RType, + Value*& RecordPtr, CharUnits Align, + Value *Func, int Lvl) Formatting looks off here and elsewhere. You should run the patch through clang-format. https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting Comment at: lib/CodeGen/CGBuiltin.cpp:938 +{ + const RecordType *RT = RType->getAs(); + ASTContext& Context = CGF.getContext(); Can use `const auto *` rather than spelling the type twice. Comment at: lib/Sema/SemaChecking.cpp:1128 + +const RecordType *RT = PtrArgType->getPointeeType()->getAs(); +if (!RT) { `const auto *` Comment at: lib/Sema/SemaChecking.cpp:1129 +const RecordType *RT = PtrArgType->getPointeeType()->getAs(); +if (!RT) { + this->Diag(PtrArg->getLocStart(), diag::err_typecheck_convert_incompatible) I'd hoist this test to avoid duplicated code. ``` if (!PtrArgType->isPointerType() || !PtrArgType->getPointeeType()->isRecordType()) ``` Comment at: lib/Sema/SemaChecking.cpp:1147-1148 + +const FunctionType *FuncType = + FnPtrArgType->getPointeeType()->getAs(); + `const auto *` Comment at: lib/Sema/SemaChecking.cpp:1158 + +if (const FunctionProtoType *FT = dyn_cast(FuncType)) { + if (!FT->isVariadic() || `const auto *` Comment at: lib/Sema/SemaChecking.cpp:1159-1160 +if (const FunctionProtoType *FT = dyn_cast(FuncType)) { + if (!FT->isVariadic() || + FT->getReturnType() != Context.IntTy) { + this->Diag(FnPtrArg->getLocStart(), diag::err_typecheck_convert_incompatible) You should also check that the first argument is `const char *` and add test cases for when it's not. Repository: rC Clang https://reviews.llvm.org/D44093 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44093: [BUILTINS] structure pretty printer
paulsemel updated this revision to Diff 138687. paulsemel added a comment. Added some tests (unit tests for almost every types) and some other tests with tricky cases. More tests are coming soon. Repository: rC Clang https://reviews.llvm.org/D44093 Files: include/clang/Basic/Builtins.def lib/CodeGen/CGBuiltin.cpp lib/Sema/SemaChecking.cpp test/CodeGen/dump-struct-builtin.c Index: test/CodeGen/dump-struct-builtin.c === --- /dev/null +++ test/CodeGen/dump-struct-builtin.c @@ -0,0 +1,270 @@ +// RUN: %clang_cc1 -emit-llvm %s -o - | FileCheck %s + +#include "Inputs/stdio.h" + +void unit1() +{ + struct U1A { +short a; + }; + + struct U1A a = { +.a = 12, + }; + + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[RES1:%[0-9]+]] = getelementptr inbounds %struct.U1A, %struct.U1A* %a, i32 0, i32 0 + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[LOAD1:%[0-9]+]] = load i16, i16* [[RES1]], + // CHECK: call i32 (i8*, ...) @printf({{.*}}, i16 [[LOAD1]]) + // CHECK: call i32 (i8*, ...) @printf( + __builtin_dump_struct(, ); +} + +void unit2() +{ + struct U2A { +unsigned short a; + }; + + struct U2A a = { +.a = 12, + }; + + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[RES1:%[0-9]+]] = getelementptr inbounds %struct.U2A, %struct.U2A* %a, i32 0, i32 0 + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[LOAD1:%[0-9]+]] = load i16, i16* [[RES1]], + // CHECK: call i32 (i8*, ...) @printf({{.*}}, i16 [[LOAD1]]) + // CHECK: call i32 (i8*, ...) @printf( + __builtin_dump_struct(, ); +} + +void unit3() +{ + struct U3A { +int a; + }; + + struct U3A a = { +.a = 12, + }; + + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[RES1:%[0-9]+]] = getelementptr inbounds %struct.U3A, %struct.U3A* %a, i32 0, i32 0 + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[LOAD1:%[0-9]+]] = load i32, i32* [[RES1]], + // CHECK: call i32 (i8*, ...) @printf({{.*}}, i32 [[LOAD1]]) + // CHECK: call i32 (i8*, ...) @printf( + __builtin_dump_struct(, ); +} + +void unit4() +{ + struct U4A { +unsigned int a; + }; + + struct U4A a = { +.a = 12, + }; + + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[RES1:%[0-9]+]] = getelementptr inbounds %struct.U4A, %struct.U4A* %a, i32 0, i32 0 + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[LOAD1:%[0-9]+]] = load i32, i32* [[RES1]], + // CHECK: call i32 (i8*, ...) @printf({{.*}}, i32 [[LOAD1]]) + // CHECK: call i32 (i8*, ...) @printf( + __builtin_dump_struct(, ); +} + +void unit5() +{ + struct U5A { +long a; + }; + + struct U5A a = { +.a = 12, + }; + + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[RES1:%[0-9]+]] = getelementptr inbounds %struct.U5A, %struct.U5A* %a, i32 0, i32 0 + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[LOAD1:%[0-9]+]] = load i64, i64* [[RES1]], + // CHECK: call i32 (i8*, ...) @printf({{.*}}, i64 [[LOAD1]]) + // CHECK: call i32 (i8*, ...) @printf( + __builtin_dump_struct(, ); +} + +void unit6() +{ + struct U6A { +unsigned long a; + }; + + struct U6A a = { +.a = 12, + }; + + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[RES1:%[0-9]+]] = getelementptr inbounds %struct.U6A, %struct.U6A* %a, i32 0, i32 0 + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[LOAD1:%[0-9]+]] = load i64, i64* [[RES1]], + // CHECK: call i32 (i8*, ...) @printf({{.*}}, i64 [[LOAD1]]) + // CHECK: call i32 (i8*, ...) @printf( + __builtin_dump_struct(, ); +} + +void unit7() +{ + struct U7A { +long long a; + }; + + struct U7A a = { +.a = 12, + }; + + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[RES1:%[0-9]+]] = getelementptr inbounds %struct.U7A, %struct.U7A* %a, i32 0, i32 0 + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[LOAD1:%[0-9]+]] = load i64, i64* [[RES1]], + // CHECK: call i32 (i8*, ...) @printf({{.*}}, i64 [[LOAD1]]) + // CHECK: call i32 (i8*, ...) @printf( + __builtin_dump_struct(, ); +} + +void unit8() +{ + struct U8A { +long long a; + }; + + struct U8A a = { +.a = 12, + }; + + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[RES1:%[0-9]+]] = getelementptr inbounds %struct.U8A, %struct.U8A* %a, i32 0, i32 0 + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[LOAD1:%[0-9]+]] = load i64, i64* [[RES1]], + // CHECK: call i32 (i8*, ...) @printf({{.*}}, i64 [[LOAD1]]) + // CHECK: call i32 (i8*, ...) @printf( + __builtin_dump_struct(, ); +} + +void unit9() +{ + struct U9A { +char a; + }; + + struct U9A a = { +.a = 'a', + }; + + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[RES1:%[0-9]+]] = getelementptr inbounds %struct.U9A, %struct.U9A* %a, i32 0, i32 0 + // CHECK: call i32 (i8*, ...) @printf( + // CHECK: [[LOAD1:%[0-9]+]] = load i8, i8* [[RES1]], + // CHECK: call i32 (i8*, ...) @printf({{.*}}, i8 [[LOAD1]]) + // CHECK: call i32 (i8*, ...) @printf( + __builtin_dump_struct(, ); +} + +void
[PATCH] D44093: [BUILTINS] structure pretty printer
paulsemel added a comment. Hi, In https://reviews.llvm.org/D44093#1034610, @lebedev.ri wrote: > BTW, as far as i can tell this still has zero test coverage (no new tests are > being added). > I'd expect to see the tests for the actual output > > - one struct per each type it is able to print > - probably some tests showing error handling, and possibly the availability > of the builtin is somehow tested, too? Sure, I am going to work on it, now that the patch seems to be kind of "Okay" for its first version ! Thanks ! Repository: rC Clang https://reviews.llvm.org/D44093 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44093: [BUILTINS] structure pretty printer
lebedev.ri added a comment. BTW, as far as i can tell this still has zero test coverage (no new tests are being added). I'd expect to see the tests for the actual output - one struct per each type it is able to print - probably some tests showing error handling, and possibly the availability of the builtin is somehow tested, too? Repository: rC Clang https://reviews.llvm.org/D44093 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44093: [BUILTINS] structure pretty printer
paulsemel updated this revision to Diff 137998. paulsemel added a comment. Applied Francis' suggestions Repository: rC Clang https://reviews.llvm.org/D44093 Files: include/clang/Basic/Builtins.def lib/CodeGen/CGBuiltin.cpp lib/Sema/SemaChecking.cpp Index: lib/Sema/SemaChecking.cpp === --- lib/Sema/SemaChecking.cpp +++ lib/Sema/SemaChecking.cpp @@ -1110,6 +1110,65 @@ // so ensure that they are declared. DeclareGlobalNewDelete(); break; + case Builtin::BI__builtin_dump_struct: { +// We first want to ensure we are called with 2 arguments +if (checkArgCount(*this, TheCall, 2)) + return ExprError(); +// Ensure that the first argument is of type 'struct XX *' +const Expr *PtrArg = TheCall->getArg(0)->IgnoreParenImpCasts(); +const QualType PtrArgType = PtrArg->getType(); +if (!PtrArgType->isPointerType()) { + this->Diag(PtrArg->getLocStart(), diag::err_typecheck_convert_incompatible) +<< PtrArgType << "\'structure pointer type\'" +<< 1 << 0 << 3 << 1 +<< PtrArgType << "\'structure pointer type\'"; + return ExprError(); +} + +const RecordType *RT = PtrArgType->getPointeeType()->getAs(); +if (!RT) { + this->Diag(PtrArg->getLocStart(), diag::err_typecheck_convert_incompatible) +<< PtrArgType << "\'structure pointer type\'" +<< 1 << 0 << 3 << 1 +<< PtrArgType << "\'structure pointer type\'"; + return ExprError(); +} +// Ensure that the second argument is of type 'FunctionType' +const Expr *FnPtrArg = TheCall->getArg(1)->IgnoreImpCasts(); +const QualType FnPtrArgType = FnPtrArg->getType(); +if (!FnPtrArgType->isPointerType()) { + this->Diag(FnPtrArg->getLocStart(), diag::err_typecheck_convert_incompatible) +<< FnPtrArgType << "\'int (*)(const char *, ...)\'" +<< 1 << 0 << 3 << 2 +<< FnPtrArgType << "\'int (*)(const char *, ...)\'"; + return ExprError(); +} + +const FunctionType *FuncType = + FnPtrArgType->getPointeeType()->getAs(); + +if (!FuncType) { + this->Diag(FnPtrArg->getLocStart(), diag::err_typecheck_convert_incompatible) +<< FnPtrArgType << "\'int (*)(const char *, ...)\'" +<< 1 << 0 << 3 << 2 +<< FnPtrArgType << "\'int (*)(const char *, ...)\'"; + return ExprError(); +} + +if (const FunctionProtoType *FT = dyn_cast(FuncType)) { + if (!FT->isVariadic() || + FT->getReturnType() != Context.IntTy) { + this->Diag(FnPtrArg->getLocStart(), diag::err_typecheck_convert_incompatible) +<< FnPtrArgType<< "\'int (*)(const char *, ...)\'" +<< 1 << 0 << 3 << 2 +<< FnPtrArgType << "\'int (*)(const char *, ...)\'"; +return ExprError(); + } +} + +TheCall->setType(Context.IntTy); +break; + } // check secure string manipulation functions where overflows // are detectable at compile time Index: lib/CodeGen/CGBuiltin.cpp === --- lib/CodeGen/CGBuiltin.cpp +++ lib/CodeGen/CGBuiltin.cpp @@ -14,6 +14,7 @@ #include "CGCXXABI.h" #include "CGObjCRuntime.h" #include "CGOpenCLRuntime.h" +#include "CGRecordLayout.h" #include "CodeGenFunction.h" #include "CodeGenModule.h" #include "ConstantEmitter.h" @@ -930,6 +931,90 @@ return RValue::get(Overflow); } +static llvm::Value *dumpRecord(CodeGenFunction , QualType RType, + Value*& RecordPtr, CharUnits Align, + Value *Func, int Lvl) +{ + const RecordType *RT = RType->getAs(); + ASTContext& Context = CGF.getContext(); + RecordDecl *RD = RT->getDecl()->getDefinition(); + ASTContext& Ctx = RD->getASTContext(); + const ASTRecordLayout = Ctx.getASTRecordLayout(RD); + std::string Pad = std::string(Lvl * 4, ' '); + + Value *GString = CGF.Builder.CreateGlobalStringPtr(RType.getAsString() + + " {\n"); + Value *Res = CGF.Builder.CreateCall(Func, {GString}); + + static llvm::DenseMapTypes; + if (Types.empty()) { +Types[Context.CharTy] = "%c"; +Types[Context.BoolTy] = "%d"; +Types[Context.IntTy] = "%d"; +Types[Context.UnsignedIntTy] = "%u"; +Types[Context.LongTy] = "%ld"; +Types[Context.UnsignedLongTy] = "%lu"; +Types[Context.LongLongTy] = "%lld"; +Types[Context.UnsignedLongLongTy] = "%llu"; +Types[Context.ShortTy] = "%hd"; +Types[Context.UnsignedShortTy] = "%hu"; +Types[Context.VoidPtrTy] = "%p"; +Types[Context.FloatTy] = "%f"; +Types[Context.DoubleTy] = "%f"; +Types[Context.LongDoubleTy] = "%Lf"; +Types[Context.getPointerType(Context.CharTy)] = "%s"; + } + + for (const auto *FD : RD->fields()) { +uint64_t Off = RL.getFieldOffset(FD->getFieldIndex()); +Off = Ctx.toCharUnitsFromBits(Off).getQuantity(); + +Value *FieldPtr = RecordPtr; +
[PATCH] D44093: [BUILTINS] structure pretty printer
thegameg added a comment. Thanks for working on this! Few remarks in the comments. Comment at: lib/CodeGen/CGBuiltin.cpp:934 +static Value *dumpRecord(CodeGenFunction , QualType RType, + Value*& RecordPtr, CharUnits Align, `llvm::Value` for consistency? Comment at: lib/CodeGen/CGBuiltin.cpp:966 +Types[Context.getPointerType(Context.CharTy)] = "%s"; +} + Indentation failed here. Comment at: lib/CodeGen/CGBuiltin.cpp:976 + FieldPtr = CGF.Builder.CreateAdd(FieldPtr, ConstantInt::get(CGF.IntPtrTy, Off)); + FieldPtr = CGF.Builder.CreateIntToPtr(FieldPtr, CGF.VoidPtrTy); +} I think you should use `getelementptr` instead of `ptrtoint` -> `inttoptr` https://llvm.org/docs/GetElementPtr.html Comment at: lib/CodeGen/CGBuiltin.cpp:997 +if (Types.find(CanonicalType) == Types.end()) +Format = Types[Context.VoidPtrTy]; +else Indentation failed here too. Comment at: lib/CodeGen/CGBuiltin.cpp:1003 +llvm::Type *ResType = CGF.ConvertType(ResPtrType); +FieldPtr = CGF.Builder.CreatePointerCast(FieldPtr, ResType); +Address FieldAddress = Address(FieldPtr, Align); If you use GEP you should be able to get rid of this cast here. Comment at: lib/CodeGen/CGBuiltin.cpp:1009 + +GString = CGF.Builder.CreateGlobalStringPtr(Format + "\n"); +TmpRes = CGF.Builder.CreateCall(Func, {GString, FieldPtr}); You can probably use `llvm::Twine` for the concatenation of `Format`: http://llvm.org/doxygen/classllvm_1_1Twine.html. Comment at: lib/Sema/SemaChecking.cpp:1159 +const FunctionProtoType *FT = dyn_cast(FuncType); +if (FT) { + if (!FT->isVariadic() || `if (const FunctionProtoType *FT = dyn_cast(FuncType))` Repository: rC Clang https://reviews.llvm.org/D44093 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44093: [BUILTINS] structure pretty printer
paulsemel updated this revision to Diff 137775. paulsemel added a comment. Added recursive type pretty-printing as suggested by Aaron. Repository: rC Clang https://reviews.llvm.org/D44093 Files: include/clang/Basic/Builtins.def lib/CodeGen/CGBuiltin.cpp lib/Sema/SemaChecking.cpp Index: lib/Sema/SemaChecking.cpp === --- lib/Sema/SemaChecking.cpp +++ lib/Sema/SemaChecking.cpp @@ -1110,6 +1110,66 @@ // so ensure that they are declared. DeclareGlobalNewDelete(); break; + case Builtin::BI__builtin_dump_struct: { +// We first want to ensure we are called with 2 arguments +if (checkArgCount(*this, TheCall, 2)) + return ExprError(); +// Ensure that the first argument is of type 'struct XX *' +const Expr *PtrArg = TheCall->getArg(0)->IgnoreParenImpCasts(); +const QualType PtrArgType = PtrArg->getType(); +if (!PtrArgType->isPointerType()) { + this->Diag(PtrArg->getLocStart(), diag::err_typecheck_convert_incompatible) +<< PtrArgType << "\'structure pointer type\'" +<< 1 << 0 << 3 << 1 +<< PtrArgType << "\'structure pointer type\'"; + return ExprError(); +} + +const RecordType *RT = PtrArgType->getPointeeType()->getAs(); +if (!RT) { + this->Diag(PtrArg->getLocStart(), diag::err_typecheck_convert_incompatible) +<< PtrArgType << "\'structure pointer type\'" +<< 1 << 0 << 3 << 1 +<< PtrArgType << "\'structure pointer type\'"; + return ExprError(); +} +// Ensure that the second argument is of type 'FunctionType' +const Expr *FnPtrArg = TheCall->getArg(1)->IgnoreImpCasts(); +const QualType FnPtrArgType = FnPtrArg->getType(); +if (!FnPtrArgType->isPointerType()) { + this->Diag(FnPtrArg->getLocStart(), diag::err_typecheck_convert_incompatible) +<< FnPtrArgType << "\'int (*)(const char *, ...)\'" +<< 1 << 0 << 3 << 2 +<< FnPtrArgType << "\'int (*)(const char *, ...)\'"; + return ExprError(); +} + +const FunctionType *FuncType = + FnPtrArgType->getPointeeType()->getAs(); + +if (!FuncType) { + this->Diag(FnPtrArg->getLocStart(), diag::err_typecheck_convert_incompatible) +<< FnPtrArgType << "\'int (*)(const char *, ...)\'" +<< 1 << 0 << 3 << 2 +<< FnPtrArgType << "\'int (*)(const char *, ...)\'"; + return ExprError(); +} + +const FunctionProtoType *FT = dyn_cast(FuncType); +if (FT) { + if (!FT->isVariadic() || + FT->getReturnType() != Context.IntTy) { + this->Diag(FnPtrArg->getLocStart(), diag::err_typecheck_convert_incompatible) +<< FnPtrArgType<< "\'int (*)(const char *, ...)\'" +<< 1 << 0 << 3 << 2 +<< FnPtrArgType << "\'int (*)(const char *, ...)\'"; +return ExprError(); + } +} + +TheCall->setType(Context.IntTy); +break; + } // check secure string manipulation functions where overflows // are detectable at compile time Index: lib/CodeGen/CGBuiltin.cpp === --- lib/CodeGen/CGBuiltin.cpp +++ lib/CodeGen/CGBuiltin.cpp @@ -14,6 +14,7 @@ #include "CGCXXABI.h" #include "CGObjCRuntime.h" #include "CGOpenCLRuntime.h" +#include "CGRecordLayout.h" #include "CodeGenFunction.h" #include "CodeGenModule.h" #include "ConstantEmitter.h" @@ -930,6 +931,93 @@ return RValue::get(Overflow); } +static Value *dumpRecord(CodeGenFunction , QualType RType, + Value*& RecordPtr, CharUnits Align, + Value *Func, int Lvl) +{ + const RecordType *RT = RType->getAs(); + ASTContext& Context = CGF.getContext(); + RecordDecl *RD = RT->getDecl()->getDefinition(); + ASTContext& Ctx = RD->getASTContext(); + const ASTRecordLayout = Ctx.getASTRecordLayout(RD); + std::string Pad = std::string(Lvl * 4, ' '); + + Value *GString = CGF.Builder.CreateGlobalStringPtr(RType.getAsString() + + " {\n"); + Value *Res = CGF.Builder.CreateCall(Func, {GString}); + + static llvm::DenseMapTypes; + if (Types.empty()) { +Types[Context.CharTy] = "%c"; +Types[Context.BoolTy] = "%d"; +Types[Context.IntTy] = "%d"; +Types[Context.UnsignedIntTy] = "%u"; +Types[Context.LongTy] = "%ld"; +Types[Context.UnsignedLongTy] = "%lu"; +Types[Context.LongLongTy] = "%lld"; +Types[Context.UnsignedLongLongTy] = "%llu"; +Types[Context.ShortTy] = "%hd"; +Types[Context.UnsignedShortTy] = "%hu"; +Types[Context.VoidPtrTy] = "%p"; +Types[Context.FloatTy] = "%f"; +Types[Context.DoubleTy] = "%f"; +Types[Context.LongDoubleTy] = "%Lf"; +Types[Context.getPointerType(Context.CharTy)] = "%s"; +} + + for (const auto *FD : RD->fields()) { +uint64_t Off = RL.getFieldOffset(FD->getFieldIndex()); +Off = Ctx.toCharUnitsFromBits(Off).getQuantity(); + +
[PATCH] D44093: [BUILTINS] structure pretty printer
paulsemel added inline comments. Comment at: lib/CodeGen/CGBuiltin.cpp:1252 + Types[getContext().getPointerType(getContext().CharTy)] = "%s"; + GENERATE_TYPE_QUALIFIERS_NO_RESTRICT(getContext().CharTy, "%s") +} aaron.ballman wrote: > paulsemel wrote: > > aaron.ballman wrote: > > > What about other types that have format specifiers (ptrdiff_t, size_t, > > > intptr_t, char16_t, etc)? > > So, for typedef, why not apply the `QualType::getCanonicalType` to our > > field type, so that we try to get rid of those intermediate typedefs ? > It should be possible to do for type aliases, because you know the canonical > type information. However, that won't work for builtin types that aren't a > typedef like `char16_t`. Sure, but in this case, the only soluntion is to determine how we want to print those builtins and add those and the static map Repository: rC Clang https://reviews.llvm.org/D44093 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44093: [BUILTINS] structure pretty printer
aaron.ballman added inline comments. Comment at: lib/CodeGen/CGBuiltin.cpp:1231 + Types[getContext().VoidPtrTy] = "%p"; + Types[getContext().FloatTy] = "%f"; + Types[getContext().DoubleTy] = "%f"; paulsemel wrote: > aaron.ballman wrote: > > paulsemel wrote: > > > aaron.ballman wrote: > > > > It's unfortunate that you cannot distinguish between `float` and > > > > `double`. Do you need to use the format specifiers exactly? > > > > > > > > What about other type information, like qualifiers or array extents? > > > > How should this handle types that do not exist in this mapping, like > > > > structure or enum types? > > > So, I've think about it. What I am going to do is that if I do not know > > > the type of the field, I am just going to print it as a pointer. I know > > > that it is not the best solution, but I think it's a okay-ish solution > > > until I implement the other types. > > > For the qualifiers, at it is printed the same way with and without those, > > > I can just add the entries in the DenseMap. > > > So, I've think about it. What I am going to do is that if I do not know > > > the type of the field, I am just going to print it as a pointer. I know > > > that it is not the best solution, but I think it's a okay-ish solution > > > until I implement the other types. > > > > Eek. That seems unfortunate. I'm thinking about very common use cases, like: > > ``` > > struct S { > > int i, j; > > float x, y; > > }; > > > > struct T { > > struct S s; > > int k; > > }; > > ``` > > Printing out `s` as a pointer seems... not particularly useful. > Yes, I see that this is true for other types that I am not handling for the > moment.. What do you think is the best behavior for those cases ? > Just not print anything and go to the next entry ? I would probably try to recurse into the contained member with another layer of `{}` around its fields; if you restructured the code such that it is a function, this recursion probably wouldn't be too bad to implement. Comment at: lib/CodeGen/CGBuiltin.cpp:1252 + Types[getContext().getPointerType(getContext().CharTy)] = "%s"; + GENERATE_TYPE_QUALIFIERS_NO_RESTRICT(getContext().CharTy, "%s") +} paulsemel wrote: > aaron.ballman wrote: > > What about other types that have format specifiers (ptrdiff_t, size_t, > > intptr_t, char16_t, etc)? > So, for typedef, why not apply the `QualType::getCanonicalType` to our field > type, so that we try to get rid of those intermediate typedefs ? It should be possible to do for type aliases, because you know the canonical type information. However, that won't work for builtin types that aren't a typedef like `char16_t`. Repository: rC Clang https://reviews.llvm.org/D44093 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44093: [BUILTINS] structure pretty printer
paulsemel updated this revision to Diff 137578. paulsemel marked 3 inline comments as done. paulsemel added a comment. Applied Aaron suggestions Repository: rC Clang https://reviews.llvm.org/D44093 Files: include/clang/Basic/Builtins.def lib/CodeGen/CGBuiltin.cpp lib/Sema/SemaChecking.cpp Index: lib/Sema/SemaChecking.cpp === --- lib/Sema/SemaChecking.cpp +++ lib/Sema/SemaChecking.cpp @@ -1110,6 +1110,66 @@ // so ensure that they are declared. DeclareGlobalNewDelete(); break; + case Builtin::BI__builtin_dump_struct: { +// We first want to ensure we are called with 2 arguments +if (checkArgCount(*this, TheCall, 2)) + return ExprError(); +// Ensure that the first argument is of type 'struct XX *' +const Expr *PtrArg = TheCall->getArg(0)->IgnoreParenImpCasts(); +const QualType PtrArgType = PtrArg->getType(); +if (!PtrArgType->isPointerType()) { + this->Diag(PtrArg->getLocStart(), diag::err_typecheck_convert_incompatible) +<< PtrArgType << "\'structure pointer type\'" +<< 1 << 0 << 3 << 1 +<< PtrArgType << "\'structure pointer type\'"; + return ExprError(); +} + +const RecordType *RT = PtrArgType->getPointeeType()->getAs(); +if (!RT) { + this->Diag(PtrArg->getLocStart(), diag::err_typecheck_convert_incompatible) +<< PtrArgType << "\'structure pointer type\'" +<< 1 << 0 << 3 << 1 +<< PtrArgType << "\'structure pointer type\'"; + return ExprError(); +} +// Ensure that the second argument is of type 'FunctionType' +const Expr *FnPtrArg = TheCall->getArg(1)->IgnoreImpCasts(); +const QualType FnPtrArgType = FnPtrArg->getType(); +if (!FnPtrArgType->isPointerType()) { + this->Diag(FnPtrArg->getLocStart(), diag::err_typecheck_convert_incompatible) +<< FnPtrArgType << "\'int (*)(const char *, ...)\'" +<< 1 << 0 << 3 << 2 +<< FnPtrArgType << "\'int (*)(const char *, ...)\'"; + return ExprError(); +} + +const FunctionType *FuncType = + FnPtrArgType->getPointeeType()->getAs(); + +if (!FuncType) { + this->Diag(FnPtrArg->getLocStart(), diag::err_typecheck_convert_incompatible) +<< FnPtrArgType << "\'int (*)(const char *, ...)\'" +<< 1 << 0 << 3 << 2 +<< FnPtrArgType << "\'int (*)(const char *, ...)\'"; + return ExprError(); +} + +const FunctionProtoType *FT = dyn_cast(FuncType); +if (FT) { + if (!FT->isVariadic() || + FT->getReturnType() != Context.IntTy) { + this->Diag(FnPtrArg->getLocStart(), diag::err_typecheck_convert_incompatible) +<< FnPtrArgType<< "\'int (*)(const char *, ...)\'" +<< 1 << 0 << 3 << 2 +<< FnPtrArgType << "\'int (*)(const char *, ...)\'"; +return ExprError(); + } +} + +TheCall->setType(Context.IntTy); +break; + } // check secure string manipulation functions where overflows // are detectable at compile time Index: lib/CodeGen/CGBuiltin.cpp === --- lib/CodeGen/CGBuiltin.cpp +++ lib/CodeGen/CGBuiltin.cpp @@ -14,6 +14,7 @@ #include "CGCXXABI.h" #include "CGObjCRuntime.h" #include "CGOpenCLRuntime.h" +#include "CGRecordLayout.h" #include "CodeGenFunction.h" #include "CodeGenModule.h" #include "ConstantEmitter.h" @@ -1196,6 +1197,84 @@ return RValue::get(ComplexVal.first); } + case Builtin::BI__builtin_dump_struct: { +Value *Func = EmitScalarExpr(E->getArg(1)->IgnoreImpCasts()); +CharUnits Arg0Align = EmitPointerWithAlignment(E->getArg(0)).getAlignment(); + +const Expr *Arg0 = E->getArg(0)->IgnoreImpCasts(); +QualType Arg0Type = Arg0->getType()->getPointeeType(); +const RecordType *RT = Arg0Type->getAs(); + +RecordDecl *RD = RT->getDecl()->getDefinition(); +ASTContext = RD->getASTContext(); +const ASTRecordLayout = Ctx.getASTRecordLayout(RD); + +Value *GString = Builder.CreateGlobalStringPtr(Arg0Type.getAsString() + + " {\n"); +Value *Res = Builder.CreateCall(Func, {GString}); + +static llvm::DenseMapTypes; +if (Types.empty()) { + Types[getContext().CharTy] = "%c"; + Types[getContext().BoolTy] = "%d"; + Types[getContext().IntTy] = "%d"; + Types[getContext().UnsignedIntTy] = "%u"; + Types[getContext().LongTy] = "%ld"; + Types[getContext().UnsignedLongTy] = "%lu"; + Types[getContext().LongLongTy] = "%lld"; + Types[getContext().UnsignedLongLongTy] = "%llu"; + Types[getContext().ShortTy] = "%hd"; + Types[getContext().UnsignedShortTy] = "%hu"; + Types[getContext().VoidPtrTy] = "%p"; + Types[getContext().FloatTy] = "%f"; + Types[getContext().DoubleTy] = "%f"; + Types[getContext().LongDoubleTy] = "%Lf"; +
[PATCH] D44093: [BUILTINS] structure pretty printer
paulsemel marked 12 inline comments as done. paulsemel added inline comments. Comment at: lib/CodeGen/CGBuiltin.cpp:1231 + Types[getContext().VoidPtrTy] = "%p"; + Types[getContext().FloatTy] = "%f"; + Types[getContext().DoubleTy] = "%f"; aaron.ballman wrote: > paulsemel wrote: > > aaron.ballman wrote: > > > It's unfortunate that you cannot distinguish between `float` and > > > `double`. Do you need to use the format specifiers exactly? > > > > > > What about other type information, like qualifiers or array extents? How > > > should this handle types that do not exist in this mapping, like > > > structure or enum types? > > So, I've think about it. What I am going to do is that if I do not know > > the type of the field, I am just going to print it as a pointer. I know > > that it is not the best solution, but I think it's a okay-ish solution > > until I implement the other types. > > For the qualifiers, at it is printed the same way with and without those, I > > can just add the entries in the DenseMap. > > So, I've think about it. What I am going to do is that if I do not know the > > type of the field, I am just going to print it as a pointer. I know that it > > is not the best solution, but I think it's a okay-ish solution until I > > implement the other types. > > Eek. That seems unfortunate. I'm thinking about very common use cases, like: > ``` > struct S { > int i, j; > float x, y; > }; > > struct T { > struct S s; > int k; > }; > ``` > Printing out `s` as a pointer seems... not particularly useful. Yes, I see that this is true for other types that I am not handling for the moment.. What do you think is the best behavior for those cases ? Just not print anything and go to the next entry ? Comment at: lib/CodeGen/CGBuiltin.cpp:1218 +Types[getContext().getConstType(type)] = format; \ +Types[getContext().getVolatileType(type)] = format; \ +Types[getContext().getConstType(getContext().getVolatileType(type))] = format; aaron.ballman wrote: > This seems insufficient, as there are other qualifiers (restrict, ObjC GC > qualifiers, etc). I think a better way to do this is to call > `QualType::getUnqualifiedType()` on the type accessing the map. Yes, I think you're totally right ! Comment at: lib/CodeGen/CGBuiltin.cpp:1252 + Types[getContext().getPointerType(getContext().CharTy)] = "%s"; + GENERATE_TYPE_QUALIFIERS_NO_RESTRICT(getContext().CharTy, "%s") +} aaron.ballman wrote: > What about other types that have format specifiers (ptrdiff_t, size_t, > intptr_t, char16_t, etc)? So, for typedef, why not apply the `QualType::getCanonicalType` to our field type, so that we try to get rid of those intermediate typedefs ? Comment at: lib/Sema/SemaChecking.cpp:1121 + this->Diag(Arg0->getLocStart(), diag::err_dump_struct_invalid_argument_type) +<< Arg0->getType() << "structure pointer type"; + return ExprError(); aaron.ballman wrote: > The string literals should be part of a `%select` in the diagnostic itself > rather than printed this way. So, I am now using an other diagnostic definition, so I am constrained to keep using string literals I think.. Comment at: lib/Sema/SemaChecking.cpp:1135 + this->Diag(Arg1->getLocStart(), diag::err_dump_struct_invalid_argument_type) +<< Arg1->getType() << "printf like function pointer type"; + return ExprError(); aaron.ballman wrote: > What is a "printf like function pointer type"? I have changed this to the actual prototype of a "printf like" function so that it's way clearer of what I am expecting ! Repository: rC Clang https://reviews.llvm.org/D44093 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44093: [BUILTINS] structure pretty printer
aaron.ballman added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:5022-5023 +def err_dump_struct_invalid_argument_type : Error< + "invalid argument of type %0; expected %1">; + Can you look to see if we have an existing diagnostic that can cover this instead? Comment at: lib/CodeGen/CGBuiltin.cpp:1231 + Types[getContext().VoidPtrTy] = "%p"; + Types[getContext().FloatTy] = "%f"; + Types[getContext().DoubleTy] = "%f"; paulsemel wrote: > aaron.ballman wrote: > > It's unfortunate that you cannot distinguish between `float` and `double`. > > Do you need to use the format specifiers exactly? > > > > What about other type information, like qualifiers or array extents? How > > should this handle types that do not exist in this mapping, like structure > > or enum types? > So, I've think about it. What I am going to do is that if I do not know the > type of the field, I am just going to print it as a pointer. I know that it > is not the best solution, but I think it's a okay-ish solution until I > implement the other types. > For the qualifiers, at it is printed the same way with and without those, I > can just add the entries in the DenseMap. > So, I've think about it. What I am going to do is that if I do not know the > type of the field, I am just going to print it as a pointer. I know that it > is not the best solution, but I think it's a okay-ish solution until I > implement the other types. Eek. That seems unfortunate. I'm thinking about very common use cases, like: ``` struct S { int i, j; float x, y; }; struct T { struct S s; int k; }; ``` Printing out `s` as a pointer seems... not particularly useful. Comment at: lib/CodeGen/CGBuiltin.cpp:1218 +Types[getContext().getConstType(type)] = format; \ +Types[getContext().getVolatileType(type)] = format; \ +Types[getContext().getConstType(getContext().getVolatileType(type))] = format; This seems insufficient, as there are other qualifiers (restrict, ObjC GC qualifiers, etc). I think a better way to do this is to call `QualType::getUnqualifiedType()` on the type accessing the map. Comment at: lib/CodeGen/CGBuiltin.cpp:1252 + Types[getContext().getPointerType(getContext().CharTy)] = "%s"; + GENERATE_TYPE_QUALIFIERS_NO_RESTRICT(getContext().CharTy, "%s") +} What about other types that have format specifiers (ptrdiff_t, size_t, intptr_t, char16_t, etc)? Comment at: lib/CodeGen/CGBuiltin.cpp:1255 + +/* field : RecordDecl::field_iterator */ +for (const auto *FD : RD->fields()) { I don't think this comment adds a lot of value. Comment at: lib/CodeGen/CGBuiltin.cpp:1269-1270 + + /* If the type is not handled yet, let's just print the data as a pointer + */ + if (Types.find(FD->getType()) == Types.end()) We generally prefer `//` style comments unless there's good reason to use `/* */`. Comment at: lib/Sema/SemaChecking.cpp:1114 + case Builtin::BI__builtin_dump_struct: { +// We check for argument number +if (checkArgCount(*this, TheCall, 2)) Comments should be grammatically correct, including punctuation (here and elsewhere). Comment at: lib/Sema/SemaChecking.cpp:1118 +// Ensure that the first argument is of type 'struct XX *' +const Expr *Arg0 = TheCall->getArg(0)->IgnoreImpCasts(); +if (!Arg0->getType()->isPointerType()) { You probably want to use `IgnoreParenImpCasts()`. Comment at: lib/Sema/SemaChecking.cpp:1119 +const Expr *Arg0 = TheCall->getArg(0)->IgnoreImpCasts(); +if (!Arg0->getType()->isPointerType()) { + this->Diag(Arg0->getLocStart(), diag::err_dump_struct_invalid_argument_type) Rather than calling `is` followed by `get`, you should just call `get` once and check the results here. Comment at: lib/Sema/SemaChecking.cpp:1121 + this->Diag(Arg0->getLocStart(), diag::err_dump_struct_invalid_argument_type) +<< Arg0->getType() << "structure pointer type"; + return ExprError(); The string literals should be part of a `%select` in the diagnostic itself rather than printed this way. Comment at: lib/Sema/SemaChecking.cpp:1132-1133 +// Ensure that the second argument is of type 'FunctionType' +const Expr *Arg1 = TheCall->getArg(1)->IgnoreImpCasts(); +if (!Arg1->getType()->isPointerType()) { + this->Diag(Arg1->getLocStart(), diag::err_dump_struct_invalid_argument_type) Same suggestions here. Also, `Arg1` and `Arg0` aren't particularly descriptive names. Can you pick names based on the semantics of the arguments rather than their position? Comment at:
[PATCH] D44093: [BUILTINS] structure pretty printer
paulsemel updated this revision to Diff 137371. paulsemel added a comment. Updated with more context. Repository: rC Clang https://reviews.llvm.org/D44093 Files: include/clang/Basic/Builtins.def include/clang/Basic/DiagnosticSemaKinds.td lib/CodeGen/CGBuiltin.cpp lib/Sema/SemaChecking.cpp Index: lib/Sema/SemaChecking.cpp === --- lib/Sema/SemaChecking.cpp +++ lib/Sema/SemaChecking.cpp @@ -1110,6 +1110,53 @@ // so ensure that they are declared. DeclareGlobalNewDelete(); break; + case Builtin::BI__builtin_dump_struct: { +// We check for argument number +if (checkArgCount(*this, TheCall, 2)) + return ExprError(); +// Ensure that the first argument is of type 'struct XX *' +const Expr *Arg0 = TheCall->getArg(0)->IgnoreImpCasts(); +if (!Arg0->getType()->isPointerType()) { + this->Diag(Arg0->getLocStart(), diag::err_dump_struct_invalid_argument_type) +<< Arg0->getType() << "structure pointer type"; + return ExprError(); +} +QualType Arg0Type = Arg0->getType()->getPointeeType(); +const RecordType *RT = Arg0Type->getAs(); +if (!RT) { + this->Diag(Arg0->getLocStart(), diag::err_dump_struct_invalid_argument_type) +<< Arg0->getType() << "structure pointer type"; + return ExprError(); +} +// Ensure that the second argument is of type 'FunctionType' +const Expr *Arg1 = TheCall->getArg(1)->IgnoreImpCasts(); +if (!Arg1->getType()->isPointerType()) { + this->Diag(Arg1->getLocStart(), diag::err_dump_struct_invalid_argument_type) +<< Arg1->getType() << "printf like function pointer type"; + return ExprError(); +} + +const FunctionType *FuncType = Arg1->getType()->getPointeeType()->getAs(); + +if (!FuncType) { + this->Diag(Arg1->getLocStart(), diag::err_dump_struct_invalid_argument_type) +<< Arg1->getType() << "printf like function pointer type"; + return ExprError(); +} + +const FunctionProtoType *FT = dyn_cast(FuncType); +if (FT) { + if (!FT->isVariadic()) { +this->Diag(Arg1->getLocStart(), diag::err_dump_struct_invalid_argument_type) + << Arg1->getType() << "printf like function pointer type"; +return ExprError(); + } +} + + +TheCall->setType(Context.IntTy); +break; + } // check secure string manipulation functions where overflows // are detectable at compile time Index: lib/CodeGen/CGBuiltin.cpp === --- lib/CodeGen/CGBuiltin.cpp +++ lib/CodeGen/CGBuiltin.cpp @@ -14,6 +14,7 @@ #include "CGCXXABI.h" #include "CGObjCRuntime.h" #include "CGOpenCLRuntime.h" +#include "CGRecordLayout.h" #include "CodeGenFunction.h" #include "CodeGenModule.h" #include "ConstantEmitter.h" @@ -1196,6 +1197,103 @@ return RValue::get(ComplexVal.first); } + case Builtin::BI__builtin_dump_struct: { +Value *Func = EmitScalarExpr(E->getArg(1)->IgnoreImpCasts()); +CharUnits Arg0Align = EmitPointerWithAlignment(E->getArg(0)).getAlignment(); + +const Expr *Arg0 = E->getArg(0)->IgnoreImpCasts(); +QualType Arg0Type = Arg0->getType()->getPointeeType(); +const RecordType *RT = Arg0Type->getAs(); + +RecordDecl *RD = RT->getDecl()->getDefinition(); +ASTContext = RD->getASTContext(); +const ASTRecordLayout = Ctx.getASTRecordLayout(RD); + +Value *GString = Builder.CreateGlobalStringPtr(Arg0Type.getAsString() + + " {\n"); +Value *Res = Builder.CreateCall(Func, {GString}); + +#define GENERATE_TYPE_QUALIFIERS_NO_RESTRICT(type, format) \ +Types[getContext().getConstType(type)] = format; \ +Types[getContext().getVolatileType(type)] = format; \ +Types[getContext().getConstType(getContext().getVolatileType(type))] = format; + +static llvm::DenseMapTypes; +if (Types.empty()) { + Types[getContext().CharTy] = "%c"; + GENERATE_TYPE_QUALIFIERS_NO_RESTRICT(getContext().CharTy, "%c") + Types[getContext().BoolTy] = "%d"; + GENERATE_TYPE_QUALIFIERS_NO_RESTRICT(getContext().BoolTy, "%d") + Types[getContext().IntTy] = "%d"; + GENERATE_TYPE_QUALIFIERS_NO_RESTRICT(getContext().IntTy, "%d") + Types[getContext().UnsignedIntTy] = "%u"; + GENERATE_TYPE_QUALIFIERS_NO_RESTRICT(getContext().UnsignedIntTy, "%u") + Types[getContext().LongTy] = "%ld"; + GENERATE_TYPE_QUALIFIERS_NO_RESTRICT(getContext().LongTy, "%ld") + Types[getContext().UnsignedLongTy] = "%lu"; + GENERATE_TYPE_QUALIFIERS_NO_RESTRICT(getContext().UnsignedLongTy, "%lu") + Types[getContext().LongLongTy] = "%lld"; + GENERATE_TYPE_QUALIFIERS_NO_RESTRICT(getContext().LongLongTy, "%lld") + Types[getContext().UnsignedLongLongTy] = "%llu"; + GENERATE_TYPE_QUALIFIERS_NO_RESTRICT(getContext().UnsignedLongLongTy, "%llu") +
[PATCH] D44093: [BUILTINS] structure pretty printer
lebedev.ri added a comment. Please always upload patches with full context (-U99) Repository: rC Clang https://reviews.llvm.org/D44093 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44093: [BUILTINS] structure pretty printer
paulsemel updated this revision to Diff 137369. paulsemel added a comment. Applied Aaron suggestion changes. Added parameters checking in Sema. Repository: rC Clang https://reviews.llvm.org/D44093 Files: include/clang/Basic/Builtins.def include/clang/Basic/DiagnosticSemaKinds.td lib/CodeGen/CGBuiltin.cpp lib/Sema/SemaChecking.cpp Index: lib/Sema/SemaChecking.cpp === --- lib/Sema/SemaChecking.cpp +++ lib/Sema/SemaChecking.cpp @@ -1110,6 +1110,53 @@ // so ensure that they are declared. DeclareGlobalNewDelete(); break; + case Builtin::BI__builtin_dump_struct: { +// We check for argument number +if (checkArgCount(*this, TheCall, 2)) + return ExprError(); +// Ensure that the first argument is of type 'struct XX *' +const Expr *Arg0 = TheCall->getArg(0)->IgnoreImpCasts(); +if (!Arg0->getType()->isPointerType()) { + this->Diag(Arg0->getLocStart(), diag::err_dump_struct_invalid_argument_type) +<< Arg0->getType() << "structure pointer type"; + return ExprError(); +} +QualType Arg0Type = Arg0->getType()->getPointeeType(); +const RecordType *RT = Arg0Type->getAs(); +if (!RT) { + this->Diag(Arg0->getLocStart(), diag::err_dump_struct_invalid_argument_type) +<< Arg0->getType() << "structure pointer type"; + return ExprError(); +} +// Ensure that the second argument is of type 'FunctionType' +const Expr *Arg1 = TheCall->getArg(1)->IgnoreImpCasts(); +if (!Arg1->getType()->isPointerType()) { + this->Diag(Arg1->getLocStart(), diag::err_dump_struct_invalid_argument_type) +<< Arg1->getType() << "printf like function pointer type"; + return ExprError(); +} + +const FunctionType *FuncType = Arg1->getType()->getPointeeType()->getAs(); + +if (!FuncType) { + this->Diag(Arg1->getLocStart(), diag::err_dump_struct_invalid_argument_type) +<< Arg1->getType() << "printf like function pointer type"; + return ExprError(); +} + +const FunctionProtoType *FT = dyn_cast(FuncType); +if (FT) { + if (!FT->isVariadic()) { +this->Diag(Arg1->getLocStart(), diag::err_dump_struct_invalid_argument_type) + << Arg1->getType() << "printf like function pointer type"; +return ExprError(); + } +} + + +TheCall->setType(Context.IntTy); +break; + } // check secure string manipulation functions where overflows // are detectable at compile time Index: lib/CodeGen/CGBuiltin.cpp === --- lib/CodeGen/CGBuiltin.cpp +++ lib/CodeGen/CGBuiltin.cpp @@ -14,6 +14,7 @@ #include "CGCXXABI.h" #include "CGObjCRuntime.h" #include "CGOpenCLRuntime.h" +#include "CGRecordLayout.h" #include "CodeGenFunction.h" #include "CodeGenModule.h" #include "ConstantEmitter.h" @@ -1196,6 +1197,103 @@ return RValue::get(ComplexVal.first); } + case Builtin::BI__builtin_dump_struct: { +Value *Func = EmitScalarExpr(E->getArg(1)->IgnoreImpCasts()); +CharUnits Arg0Align = EmitPointerWithAlignment(E->getArg(0)).getAlignment(); + +const Expr *Arg0 = E->getArg(0)->IgnoreImpCasts(); +QualType Arg0Type = Arg0->getType()->getPointeeType(); +const RecordType *RT = Arg0Type->getAs(); + +RecordDecl *RD = RT->getDecl()->getDefinition(); +ASTContext = RD->getASTContext(); +const ASTRecordLayout = Ctx.getASTRecordLayout(RD); + +Value *GString = Builder.CreateGlobalStringPtr(Arg0Type.getAsString() + + " {\n"); +Value *Res = Builder.CreateCall(Func, {GString}); + +#define GENERATE_TYPE_QUALIFIERS_NO_RESTRICT(type, format) \ +Types[getContext().getConstType(type)] = format; \ +Types[getContext().getVolatileType(type)] = format; \ +Types[getContext().getConstType(getContext().getVolatileType(type))] = format; + +static llvm::DenseMapTypes; +if (Types.empty()) { + Types[getContext().CharTy] = "%c"; + GENERATE_TYPE_QUALIFIERS_NO_RESTRICT(getContext().CharTy, "%c") + Types[getContext().BoolTy] = "%d"; + GENERATE_TYPE_QUALIFIERS_NO_RESTRICT(getContext().BoolTy, "%d") + Types[getContext().IntTy] = "%d"; + GENERATE_TYPE_QUALIFIERS_NO_RESTRICT(getContext().IntTy, "%d") + Types[getContext().UnsignedIntTy] = "%u"; + GENERATE_TYPE_QUALIFIERS_NO_RESTRICT(getContext().UnsignedIntTy, "%u") + Types[getContext().LongTy] = "%ld"; + GENERATE_TYPE_QUALIFIERS_NO_RESTRICT(getContext().LongTy, "%ld") + Types[getContext().UnsignedLongTy] = "%lu"; + GENERATE_TYPE_QUALIFIERS_NO_RESTRICT(getContext().UnsignedLongTy, "%lu") + Types[getContext().LongLongTy] = "%lld"; + GENERATE_TYPE_QUALIFIERS_NO_RESTRICT(getContext().LongLongTy, "%lld") + Types[getContext().UnsignedLongLongTy] = "%llu"; +
[PATCH] D44093: [BUILTINS] structure pretty printer
paulsemel marked 2 inline comments as done. paulsemel added inline comments. Comment at: lib/CodeGen/CGBuiltin.cpp:1206 +QualType Arg0Type = Arg0->getType()->getPointeeType(); +const RecordType *RT = Arg0Type->getAs(); + aaron.ballman wrote: > You can use `const auto *` here because the type is spelled out in the > initializer. After thinking about it, I find it clearer to spell the full type when possible (that's also probably because I'm used to code in C :) ) Comment at: lib/CodeGen/CGBuiltin.cpp:1231 + Types[getContext().VoidPtrTy] = "%p"; + Types[getContext().FloatTy] = "%f"; + Types[getContext().DoubleTy] = "%f"; aaron.ballman wrote: > It's unfortunate that you cannot distinguish between `float` and `double`. Do > you need to use the format specifiers exactly? > > What about other type information, like qualifiers or array extents? How > should this handle types that do not exist in this mapping, like structure or > enum types? So, I've think about it. What I am going to do is that if I do not know the type of the field, I am just going to print it as a pointer. I know that it is not the best solution, but I think it's a okay-ish solution until I implement the other types. For the qualifiers, at it is printed the same way with and without those, I can just add the entries in the DenseMap. Repository: rC Clang https://reviews.llvm.org/D44093 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44093: [BUILTINS] structure pretty printer
aaron.ballman added inline comments. Comment at: lib/CodeGen/CGBuiltin.cpp:1208 + +assert(RT && "The first argument must be a record type"); + paulsemel wrote: > aaron.ballman wrote: > > I don't see anything enforcing this constraint, so users are likely to hit > > this assertion rather than a compile error. > I actually didn't manage to enforce this in the builtin declaration as we can > only declare simple types (I am probably missing something) I think this builtin will require custom type checking (so its signature needs a `t`), and then you can add your code to `Sema::CheckBuiltinFunctionCall()` to do the actual semantic checking. Comment at: lib/CodeGen/CGBuiltin.cpp:1258 + + //Need to handle bitfield here + paulsemel wrote: > aaron.ballman wrote: > > Are you intending to implement this as part of this functionality? > Yes, my goal is to be able to dump the bitfields correctly, particularly if > the structure is packed (for dumping a GDT for example). > I just didn't manage to do it properly for the moment. Okay, this should probably be a FIXME comment if you don't intend to handle it in the initial patch. It should also have a test case with some comments about the test behavior. Comment at: lib/CodeGen/CGBuiltin.cpp:1206 +QualType Arg0Type = Arg0->getType()->getPointeeType(); +const RecordType *RT = Arg0Type->getAs(); + You can use `const auto *` here because the type is spelled out in the initializer. Comment at: lib/CodeGen/CGBuiltin.cpp:1211 +RecordDecl *RD = RT->getDecl()->getDefinition(); +ASTContext = RD->getASTContext(); +const ASTRecordLayout = ASTContext.getASTRecordLayout(RD); Please don't name the variable the same name as a type. Comment at: lib/CodeGen/CGBuiltin.cpp:1231 + Types[getContext().VoidPtrTy] = "%p"; + Types[getContext().FloatTy] = "%f"; + Types[getContext().DoubleTy] = "%f"; It's unfortunate that you cannot distinguish between `float` and `double`. Do you need to use the format specifiers exactly? What about other type information, like qualifiers or array extents? How should this handle types that do not exist in this mapping, like structure or enum types? Comment at: lib/CodeGen/CGBuiltin.cpp:1238 +/* field : RecordDecl::field_iterator */ +for (auto *FD : RD->fields()) { + uint64_t Off = RL.getFieldOffset(FD->getFieldIndex()); `const auto *` Repository: rC Clang https://reviews.llvm.org/D44093 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44093: [BUILTINS] structure pretty printer
paulsemel updated this revision to Diff 137141. paulsemel added a comment. Applied Aaron change suggestions Repository: rC Clang https://reviews.llvm.org/D44093 Files: include/clang/Basic/Builtins.def lib/CodeGen/CGBuiltin.cpp Index: lib/CodeGen/CGBuiltin.cpp === --- lib/CodeGen/CGBuiltin.cpp +++ lib/CodeGen/CGBuiltin.cpp @@ -14,6 +14,7 @@ #include "CGCXXABI.h" #include "CGObjCRuntime.h" #include "CGOpenCLRuntime.h" +#include "CGRecordLayout.h" #include "CodeGenFunction.h" #include "CodeGenModule.h" #include "ConstantEmitter.h" @@ -1196,6 +1197,80 @@ return RValue::get(ComplexVal.first); } + case Builtin::BI__builtin_dump_struct: { +Value *Func = EmitScalarExpr(E->getArg(1)->IgnoreImpCasts()); +CharUnits Arg0Align = EmitPointerWithAlignment(E->getArg(0)).getAlignment(); + +const Expr *Arg0 = E->getArg(0)->IgnoreImpCasts(); +QualType Arg0Type = Arg0->getType()->getPointeeType(); +const RecordType *RT = Arg0Type->getAs(); + +assert(RT && "The first argument must be a record type"); + +RecordDecl *RD = RT->getDecl()->getDefinition(); +ASTContext = RD->getASTContext(); +const ASTRecordLayout = ASTContext.getASTRecordLayout(RD); + +Value *GString = Builder.CreateGlobalStringPtr(Arg0Type.getAsString() + + " {\n"); +Value *Res = Builder.CreateCall(Func, {GString}); + +static llvm::DenseMapTypes; +if (Types.empty()) { + Types[getContext().CharTy] = "%c"; + Types[getContext().BoolTy] = "%d"; + Types[getContext().IntTy] = "%d"; + Types[getContext().UnsignedIntTy] = "%u"; + Types[getContext().LongTy] = "%ld"; + Types[getContext().UnsignedLongTy] = "%lu"; + Types[getContext().LongLongTy] = "%lld"; + Types[getContext().UnsignedLongLongTy] = "%llu"; + Types[getContext().ShortTy] = "%hd"; + Types[getContext().UnsignedShortTy] = "%hu"; + Types[getContext().VoidPtrTy] = "%p"; + Types[getContext().FloatTy] = "%f"; + Types[getContext().DoubleTy] = "%f"; + Types[getContext().LongDoubleTy] = "%Lf"; + Types[getContext().getPointerType(getContext().CharTy)] = "%s"; +} + +/* field : RecordDecl::field_iterator */ +for (auto *FD : RD->fields()) { + uint64_t Off = RL.getFieldOffset(FD->getFieldIndex()); + Off = ASTContext.toCharUnitsFromBits(Off).getQuantity(); + + Value *FieldPtr = EmitScalarExpr(E->getArg(0)); + if (Off) { +FieldPtr = Builder.CreatePtrToInt(FieldPtr, IntPtrTy); +FieldPtr = Builder.CreateAdd(FieldPtr, ConstantInt::get(IntPtrTy, Off)); +FieldPtr = Builder.CreateIntToPtr(FieldPtr, VoidPtrTy); + } + std::string Format = FD->getType().getAsString() + std::string(" ") + +FD->getNameAsString() + " : "; + + Format += Types[FD->getType()]; + + QualType ResPtrType = getContext().getPointerType(FD->getType()); + llvm::Type *ResType = ConvertType(ResPtrType); + FieldPtr = Builder.CreatePointerCast(FieldPtr, ResType); + Address FieldAddress = Address(FieldPtr, Arg0Align); + FieldPtr = Builder.CreateLoad(FieldAddress); + + // Need to handle bitfield here + + GString = Builder.CreateGlobalStringPtr(Format + "\n"); + Value *TmpRes = Builder.CreateCall(Func, {GString, FieldPtr}); + Res = Builder.CreateAdd(Res, TmpRes); +} + +std::string Format = "}\n"; +GString = Builder.CreateGlobalStringPtr(Format); +Value *TmpRes = Builder.CreateCall(Func, {GString}); +Res = Builder.CreateAdd(Res, TmpRes); + +return RValue::get(Res); + } + case Builtin::BI__builtin_cimag: case Builtin::BI__builtin_cimagf: case Builtin::BI__builtin_cimagl: Index: include/clang/Basic/Builtins.def === --- include/clang/Basic/Builtins.def +++ include/clang/Basic/Builtins.def @@ -1374,6 +1374,7 @@ BUILTIN(__builtin_operator_new, "v*z", "c") BUILTIN(__builtin_operator_delete, "vv*", "n") BUILTIN(__builtin_char_memchr, "c*cC*iz", "n") +BUILTIN(__builtin_dump_struct, "ivC*v*", "n") // Safestack builtins BUILTIN(__builtin___get_unsafe_stack_start, "v*", "Fn") ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44093: [BUILTINS] structure pretty printer
paulsemel marked 4 inline comments as done. paulsemel added inline comments. Comment at: lib/CodeGen/CGBuiltin.cpp:1208 + +assert(RT && "The first argument must be a record type"); + aaron.ballman wrote: > I don't see anything enforcing this constraint, so users are likely to hit > this assertion rather than a compile error. I actually didn't manage to enforce this in the builtin declaration as we can only declare simple types (I am probably missing something) Comment at: lib/CodeGen/CGBuiltin.cpp:1258 + + //Need to handle bitfield here + aaron.ballman wrote: > Are you intending to implement this as part of this functionality? Yes, my goal is to be able to dump the bitfields correctly, particularly if the structure is packed (for dumping a GDT for example). I just didn't manage to do it properly for the moment. Repository: rC Clang https://reviews.llvm.org/D44093 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44093: [BUILTINS] structure pretty printer
aaron.ballman added reviewers: echristo, rsmith. aaron.ballman added a comment. Adding some reviewers. One thing that's missing from this patch are test cases that demonstrate both the semantic checking (builtin accepts proper args, rejects improper ones, etc) and output. Comment at: lib/CodeGen/CGBuiltin.cpp:1204-1205 + +const Expr *e = E->getArg(0)->IgnoreImpCasts(); +QualType type = e->getType()->getPointeeType(); +const RecordType *RT = type->getAs(); These declarations do not meet our coding style guidelines -- you should pick some new names (elsewhere as well). Comment at: lib/CodeGen/CGBuiltin.cpp:1208 + +assert(RT && "The first argument must be a record type"); + I don't see anything enforcing this constraint, so users are likely to hit this assertion rather than a compile error. Comment at: lib/CodeGen/CGBuiltin.cpp:1211 +RecordDecl *RD = RT->getDecl()->getDefinition(); +auto = RD->getASTContext(); +const ASTRecordLayout = ASTContext.getASTRecordLayout(RD); Please don't use `auto` when the type is not explicitly spelled out in the initializer. Comment at: lib/CodeGen/CGBuiltin.cpp:1218-1232 +Types[getContext().CharTy] = "%c"; +Types[getContext().BoolTy] = "%d"; +Types[getContext().IntTy] = "%d"; +Types[getContext().UnsignedIntTy] = "%u"; +Types[getContext().LongTy] = "%ld"; +Types[getContext().UnsignedLongTy] = "%lu"; +Types[getContext().LongLongTy] = "%lld"; These should only be set up one time rather than each time someone calls the builtin. Comment at: lib/CodeGen/CGBuiltin.cpp:1235-1236 +/* field : RecordDecl::field_iterator */ +for (auto field = RD->field_begin(); field != RD->field_end(); field++) +{ + uint64_t off = RL.getFieldOffset(field->getFieldIndex()); Can be replaced with a range-based for loop over `fields()`. Also, the formatting of the braces doesn't match the coding standard (happens elsewhere as well) -- you should run your patch through clang-format: https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting Comment at: lib/CodeGen/CGBuiltin.cpp:1258 + + //Need to handle bitfield here + Are you intending to implement this as part of this functionality? Repository: rC Clang https://reviews.llvm.org/D44093 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44093: [BUILTINS] structure pretty printer
paulsemel created this revision. paulsemel added a reviewer: aaron.ballman. The purpose of this new builtin is to be able to pretty print any structure at runtime. This might be really useful when debugging is not an option or is fastidious (like for kernel development). Repository: rC Clang https://reviews.llvm.org/D44093 Files: include/clang/Basic/Builtins.def lib/CodeGen/CGBuiltin.cpp Index: lib/CodeGen/CGBuiltin.cpp === --- lib/CodeGen/CGBuiltin.cpp +++ lib/CodeGen/CGBuiltin.cpp @@ -17,6 +17,7 @@ #include "CodeGenFunction.h" #include "CodeGenModule.h" #include "ConstantEmitter.h" +#include "CGRecordLayout.h" #include "TargetInfo.h" #include "clang/AST/ASTContext.h" #include "clang/AST/Decl.h" @@ -1196,6 +1197,79 @@ return RValue::get(ComplexVal.first); } + case Builtin::BI__builtin_dump_struct: { +Value *Func = EmitScalarExpr(E->getArg(1)->IgnoreImpCasts()); +CharUnits Arg0Align = EmitPointerWithAlignment(E->getArg(0)).getAlignment(); + +const Expr *e = E->getArg(0)->IgnoreImpCasts(); +QualType type = e->getType()->getPointeeType(); +const RecordType *RT = type->getAs(); + +assert(RT && "The first argument must be a record type"); + +RecordDecl *RD = RT->getDecl()->getDefinition(); +auto = RD->getASTContext(); +const ASTRecordLayout = ASTContext.getASTRecordLayout(RD); + +Value *string = Builder.CreateGlobalStringPtr(type.getAsString() + " {\n"); +Value *res = Builder.CreateCall(Func, {string}); + +static llvm::DenseMapTypes; +Types[getContext().CharTy] = "%c"; +Types[getContext().BoolTy] = "%d"; +Types[getContext().IntTy] = "%d"; +Types[getContext().UnsignedIntTy] = "%u"; +Types[getContext().LongTy] = "%ld"; +Types[getContext().UnsignedLongTy] = "%lu"; +Types[getContext().LongLongTy] = "%lld"; +Types[getContext().UnsignedLongLongTy] = "%llu"; +Types[getContext().ShortTy] = "%hd"; +Types[getContext().UnsignedShortTy] = "%hu"; +Types[getContext().VoidPtrTy] = "%p"; +Types[getContext().FloatTy] = "%f"; +Types[getContext().DoubleTy] = "%f"; +Types[getContext().LongDoubleTy] = "%Lf"; +Types[getContext().getPointerType(getContext().CharTy)] = "%s"; + +/* field : RecordDecl::field_iterator */ +for (auto field = RD->field_begin(); field != RD->field_end(); field++) +{ + uint64_t off = RL.getFieldOffset(field->getFieldIndex()); + off = ASTContext.toCharUnitsFromBits(off).getQuantity(); + + Value *FieldPtr = EmitScalarExpr(E->getArg(0)); + if (off) + { +FieldPtr = Builder.CreatePtrToInt(FieldPtr, IntPtrTy); +FieldPtr = Builder.CreateAdd(FieldPtr, ConstantInt::get(IntPtrTy, off)); +FieldPtr = Builder.CreateIntToPtr(FieldPtr, VoidPtrTy); + } + std::string str = field->getType().getAsString() + std::string(" ") + +field->getNameAsString() + " : "; + + str += Types[field->getType()]; + + QualType ResPtrType = getContext().getPointerType(field->getType()); + llvm::Type *ResType = ConvertType(ResPtrType); + FieldPtr = Builder.CreatePointerCast(FieldPtr, ResType); + Address address = Address(FieldPtr, Arg0Align); + FieldPtr = Builder.CreateLoad(address); + + //Need to handle bitfield here + + string = Builder.CreateGlobalStringPtr(str + "\n"); + Value *tmp = Builder.CreateCall(Func, {string, FieldPtr}); + res = Builder.CreateAdd(res, tmp); +} + +std::string str = "}\n"; +string = Builder.CreateGlobalStringPtr(str); +Value *tmp = Builder.CreateCall(Func, {string}); +res = Builder.CreateAdd(res, tmp); + +return RValue::get(res); + } + case Builtin::BI__builtin_cimag: case Builtin::BI__builtin_cimagf: case Builtin::BI__builtin_cimagl: Index: include/clang/Basic/Builtins.def === --- include/clang/Basic/Builtins.def +++ include/clang/Basic/Builtins.def @@ -1374,6 +1374,7 @@ BUILTIN(__builtin_operator_new, "v*z", "c") BUILTIN(__builtin_operator_delete, "vv*", "n") BUILTIN(__builtin_char_memchr, "c*cC*iz", "n") +BUILTIN(__builtin_dump_struct, "ivC*v*", "n") // Safestack builtins BUILTIN(__builtin___get_unsafe_stack_start, "v*", "Fn") ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits