[PATCH] D44093: [BUILTINS] structure pretty printer

2018-04-10 Thread Aaron Ballman via Phabricator via cfe-commits
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

2018-04-10 Thread Paul Semel via Phabricator via cfe-commits
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

2018-04-10 Thread Aaron Ballman via Phabricator via cfe-commits
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

2018-04-10 Thread Paul Semel via Phabricator via cfe-commits
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

2018-04-10 Thread Alexander Richardson via Phabricator via cfe-commits
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

2018-04-10 Thread Alexander Richardson via Phabricator via cfe-commits
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

2018-04-10 Thread Roman Lebedev via Phabricator via cfe-commits
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

2018-04-10 Thread Paul Semel via Phabricator via cfe-commits
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

2018-04-10 Thread Paul Semel via Phabricator via cfe-commits
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

2018-04-10 Thread Paul Semel via Phabricator via cfe-commits
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

2018-04-10 Thread Aaron Ballman via Phabricator via cfe-commits
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

2018-04-10 Thread Paul Semel via Phabricator via cfe-commits
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

2018-04-10 Thread Aaron Ballman via Phabricator via cfe-commits
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

2018-04-10 Thread Paul Semel via Phabricator via cfe-commits
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

2018-04-10 Thread Aaron Ballman via Phabricator via cfe-commits
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

2018-04-10 Thread Paul Semel via Phabricator via cfe-commits
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

2018-04-10 Thread Alexander Richardson via Phabricator via cfe-commits
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

2018-04-10 Thread Paul Semel via Phabricator via cfe-commits
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

2018-04-10 Thread Alexander Richardson via Phabricator via cfe-commits
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

2018-04-10 Thread Aaron Ballman via Phabricator via cfe-commits
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

2018-04-09 Thread Paul Semel via Phabricator via cfe-commits
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

2018-04-09 Thread Paul Semel via Phabricator via cfe-commits
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

2018-04-09 Thread Aaron Ballman via Phabricator via cfe-commits
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

2018-04-09 Thread Paul Semel via Phabricator via cfe-commits
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

2018-04-09 Thread Paul Semel via Phabricator via cfe-commits
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

2018-04-09 Thread Aaron Ballman via Phabricator via cfe-commits
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

2018-03-29 Thread Paul Semel via Phabricator via cfe-commits
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

2018-03-29 Thread Paul Semel via Phabricator via cfe-commits
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

2018-03-20 Thread Aaron Ballman via Phabricator via cfe-commits
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

2018-03-17 Thread Paul Semel via Phabricator via cfe-commits
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

2018-03-16 Thread Aaron Ballman via Phabricator via cfe-commits
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

2018-03-16 Thread Paul Semel via Phabricator via cfe-commits
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

2018-03-12 Thread Paul Semel via Phabricator via cfe-commits
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

2018-03-12 Thread Roman Lebedev via Phabricator via cfe-commits
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

2018-03-12 Thread Paul Semel via Phabricator via cfe-commits
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::DenseMap Types;
+  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

2018-03-09 Thread Francis Visoiu Mistrih via Phabricator via cfe-commits
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

2018-03-09 Thread Paul Semel via Phabricator via cfe-commits
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::DenseMap Types;
+  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

2018-03-09 Thread Paul Semel via Phabricator via cfe-commits
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

2018-03-08 Thread Aaron Ballman via Phabricator via cfe-commits
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

2018-03-08 Thread Paul Semel via Phabricator via cfe-commits
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::DenseMap Types;
+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

2018-03-08 Thread Paul Semel via Phabricator via cfe-commits
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

2018-03-07 Thread Aaron Ballman via Phabricator via cfe-commits
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

2018-03-07 Thread Paul Semel via Phabricator via cfe-commits
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::DenseMap Types;
+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

2018-03-07 Thread Roman Lebedev via Phabricator via cfe-commits
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

2018-03-07 Thread Paul Semel via Phabricator via cfe-commits
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::DenseMap Types;
+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

2018-03-07 Thread Paul Semel via Phabricator via cfe-commits
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

2018-03-06 Thread Aaron Ballman via Phabricator via cfe-commits
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

2018-03-06 Thread Paul Semel via Phabricator via cfe-commits
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::DenseMap Types;
+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

2018-03-06 Thread Paul Semel via Phabricator via cfe-commits
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

2018-03-05 Thread Aaron Ballman via Phabricator via cfe-commits
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

2018-03-05 Thread Paul Semel via Phabricator via cfe-commits
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::DenseMap Types;
+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