[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:7671
+ AllArgs, CallType))
+return true;
+

rjmccall wrote:
> yihanaa wrote:
> > rjmccall wrote:
> > > yihanaa wrote:
> > > > rjmccall wrote:
> > > > > You can just pull the argument expressions out of the `CallExpr`; you 
> > > > > don't need to call `GatherArgumentsForCall`.
> > > > > You can just pull the argument expressions out of the `CallExpr`; you 
> > > > > don't need to call `GatherArgumentsForCall`.
> > > > 
> > > > This GatherArgumentsForCall  was used to do the common sema checking 
> > > > and emit warning, like './main.cpp:5:40: warning: passing 'volatile 
> > > > char *' to parameter of type 'const void *' discards qualifiers 
> > > > [-Wincompatible-pointer-types-discards-qualifiers]' hahaha, for this is 
> > > > a common case, I also think  GatherArgumentsForCall is not a good choice
> > > > , so I try to find a replacement, e.g. ImpCastExprToType or other ways, 
> > > > what do you think about?
> > > `convertArgumentToType` should trigger any useful warnings in the second 
> > > and third arguments.  For the first, I don't actually think there are any 
> > > warnings we care about.
> > I'm sorry John, I can't find `convertArgumentToType ` in clang, did you 
> > mean `ConvertArgumentsForCall` or `ImpCastExprToType`. we can't use 
> > `ConvertArgumentsForCall `, because `ConvertArgumentsForCall ` has checked 
> > if current CallExpr calling a builtin function with custom sema checking, 
> > it will do nothing and return.
> Oh, sorry, it's a helper function in Apple's fork that we added for the 
> ptrauth builtins but haven't upstreamed yet.  Feel free to add it yourself, 
> at the top of the file right after `checkArgCount`:
> 
> ```
> static bool convertArgumentToType(Sema , Expr *, QualType Ty) {
>   if (Value->isTypeDependent())
> return false;
> 
>   InitializedEntity Entity =
> InitializedEntity::InitializeParameter(S.Context, Ty, false);
>   ExprResult Result =
> S.PerformCopyInitialization(Entity, SourceLocation(), Value);
>   if (Result.isInvalid())
> return true;
>   Value = Result.get();
>   return false;
> }
> ```
wow,cool! Thanks John


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131979/new/

https://reviews.llvm.org/D131979

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:7671
+ AllArgs, CallType))
+return true;
+

rjmccall wrote:
> yihanaa wrote:
> > rjmccall wrote:
> > > You can just pull the argument expressions out of the `CallExpr`; you 
> > > don't need to call `GatherArgumentsForCall`.
> > > You can just pull the argument expressions out of the `CallExpr`; you 
> > > don't need to call `GatherArgumentsForCall`.
> > 
> > This GatherArgumentsForCall  was used to do the common sema checking and 
> > emit warning, like './main.cpp:5:40: warning: passing 'volatile char *' to 
> > parameter of type 'const void *' discards qualifiers 
> > [-Wincompatible-pointer-types-discards-qualifiers]' hahaha, for this is a 
> > common case, I also think  GatherArgumentsForCall is not a good choice
> > , so I try to find a replacement, e.g. ImpCastExprToType or other ways, 
> > what do you think about?
> `convertArgumentToType` should trigger any useful warnings in the second and 
> third arguments.  For the first, I don't actually think there are any 
> warnings we care about.
I'm sorry John, I can't find `convertArgumentToType ` in clang, did you mean 
`ConvertArgumentsForCall` or `ImpCastExprToType`. we can't use 
`ConvertArgumentsForCall `, because `ConvertArgumentsForCall ` has checked if 
current CallExpr calling a builtin function with custom sema checking, it will 
do nothing and return.



Comment at: clang/lib/Sema/SemaChecking.cpp:7684
+  if (FirstArgResult.isInvalid())
+return true;
+

rjmccall wrote:
> There is in fact a `DefaultFunctionArrayLvalueConversion` method you can use 
> to do all of this at once, and you don't need to explicitly check for a 
> function or array type first.
+1


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131979/new/

https://reviews.llvm.org/D131979

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

Thanks for your review John, I'll try to fix these problem later which you 
point out.




Comment at: clang/lib/Sema/SemaChecking.cpp:7659
+   << 0 << 2 << TheCall->getNumArgs() << Callee->getSourceRange();
+  }
+

rjmccall wrote:
> It looks like this should be `if (NumArgs < 2)`.
> 
> We actually have helper functions in this file for doing this kind of 
> arg-count checking, but it looks like we don't have one that takes a range.  
> Could you just generalize `checkArgCount` so that it takes a min and max arg 
> count?  You can make the existing function call your generalization.
It's my fault, I'll fix it, thanks!



Comment at: clang/lib/Sema/SemaChecking.cpp:7671
+ AllArgs, CallType))
+return true;
+

rjmccall wrote:
> You can just pull the argument expressions out of the `CallExpr`; you don't 
> need to call `GatherArgumentsForCall`.
> You can just pull the argument expressions out of the `CallExpr`; you don't 
> need to call `GatherArgumentsForCall`.

This GatherArgumentsForCall  was used to do the common sema checking and emit 
warning, like './main.cpp:5:40: warning: passing 'volatile char *' to parameter 
of type 'const void *' discards qualifiers 
[-Wincompatible-pointer-types-discards-qualifiers]' hahaha, for this is a 
common case, I also think  GatherArgumentsForCall is not a good choice
, so I try to find a replacement, e.g. ImpCastExprToType or other ways, what do 
you think about?



Comment at: clang/lib/Sema/SemaChecking.cpp:7690
+  // cast instruction which cast type from user-written-type to VoidPtr in
+  // CodeGen.
+  AllArgs[0] = FirstArgResult.get();

rjmccall wrote:
> This comment doesn't mean anything in the context of the current 
> implementation.  If you want to keep something like it (you really don't need 
> to), you could have a comment at the top explaining that we use custom 
> type-checking because it accepts an arbitrary pointer type as the first 
> argument.
+1



Comment at: clang/lib/Sema/SemaChecking.cpp:7695
+TheCall->setArg(i, AllArgs[i]);
+  TheCall->computeDependence();
+

rjmccall wrote:
> This is really just `TheCall->setArg(i, FirstArgResult.get())`.
+1



Comment at: clang/lib/Sema/SemaChecking.cpp:7701
   // We can't check the value of a dependent argument.
-  if (!Arg->isTypeDependent() && !Arg->isValueDependent()) {
+  if (!SecondArg->isTypeDependent() && !SecondArg->isValueDependent()) {
 llvm::APSInt Result;

rjmccall wrote:
> Type-dependence implies value-dependence, so you can just check for the 
> latter.
> 
> You should call `convertArgumentToType` with `size_t` in this block, just in 
> case the argument is something like an l-value reference to a `const` integer 
> variable.
+1



Comment at: clang/lib/Sema/SemaChecking.cpp:7719
+Context, Context.getSizeType(), false);
+ThirdArg = PerformCopyInitialization(Entity, SourceLocation(), ThirdArg);
+if (ThirdArg.isInvalid())

rjmccall wrote:
> You can use `convertArgumentToType` here.
+1


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131979/new/

https://reviews.llvm.org/D131979

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

Now that parameter checking is generic, I think it would be better to use both 
common seam checking and custom sema checking, like Diff 456142  
(https://reviews.llvm.org/differential/diff/456142/), what do you think about?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131979/new/

https://reviews.llvm.org/D131979

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

In D131979#3756474 , @rjmccall wrote:

> In D131979#3756067 , @yihanaa wrote:
>
>> In D131979#3756006 , @rjmccall 
>> wrote:
>>
>>> In D131979#3753358 , @yihanaa 
>>> wrote:
>>>
 In D131979#3752208 , @rjmccall 
 wrote:

> From the test case, it looks like the builtin just ignores pointers to 
> volatile types, which should be preserved by the conversions you're now 
> doing in Sema.  That is, you should be able to just check 
> `Ptr->getType()->castAs()->getPointeeType().isVolatile()`.
>
> It would be different if it ignored pointers loaded out of volatile 
> l-values or something, but that's explicitly not what it's doing.

 Thanks for your tips John, form the test case global constants and 
 compiler-rt ubsan class TypeDescriptor, it looks like pass an type 
 description to __ubsan_handle_alignment_assumption, so, we have to use 
 getSubExprAsWritten to get the origin(user written arg type) type 
 description of 1st arg, but not 'const void *', for example,   in 
 catch-alignment-assumption-builtin_assume_aligned-three-params.cpp, the 
 1st arg type description is 'char **', but if we don't use 
 getSubExprAsWritten, all the type description will be 'const void *'
>>>
>>> The version of this patch which changed the builtin to use custom 
>>> type-checking didn't need to worry about this because doing that will stop 
>>> Sema from converting to `const void *`.  You seem to have taken all of that 
>>>  code out and reverted to an earlier approach, though, and I don't see 
>>> anything in this review explaining why.
>>>
>>> Also, please don't ping for review on Monday when you submitted a new 
>>> version of the patch on Saturday.  Not only is that a very short period to 
>>> be demanding review during, but a lot of reviewers keep up with review as 
>>> part of their normal workday and cannot be expected to review over the 
>>> weekend.
>>
>> Thanks for your reply John, the patch of the previous version which will 
>> emit an error when 1st arg is volatile-qualified, we're  worried about the 
>> shift to emit an unconditional error on `volatile`. So, I'll try to separate 
>> that into its own patch and just fix the bug in this one. and I'm so sorry 
>> about have 'pin' too often, this is due to time zone, I will pay attention 
>> to it later, and thanks for your tips!
>
> Hmm.  I think you can take the previous version of the patch and either (1) 
> remove the place where it explicitly emits an error or (2) make it only emit 
> an error in C++ mode.
>
> I think the type descriptor for something that starts as an array reference 
> should probably be a pointer, i.e. you should apply array decay and then 
> derive the type from that.  That's one of the things that'll just fall out 
> naturally if you do what the previous version of the patch was doing.

Thanks for your suggestions John. As far as I know,  ‘warning: passing 
'volatile char *' to parameter of type 'const void *' discards qualifiers’ is a 
common case, so I use GatherArgumentsForCall to handle these common cases.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131979/new/

https://reviews.llvm.org/D131979

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

[Wang Yihan via Phabricator via cfe-commits]

yihanaa updated this revision to Diff 456646.
yihanaa added a comment.

Use custom seam checking on __builtin_assume_aligned.

C++ not allow implicit cast from volatile void * to const void *, but C allowed 
and only emit a warning. This is a general case, volatile in 
__builtin_assume_aligned just use to ignore generate 'call void 
@__ubsan_handle_alignment_assumption' in CodeGen. So, I use 
GatherArgumentsForCall to handle the common case(e.g. emit diag message) except 
1st arg, we ignore the implicit cast from user-written-type to 'const void *' 
on the 1st arg, because CodeGen need user-written-type to generate correct 
TypeDescriptor (this class in compiler-rt/UBSan), then, clang will emit cast 
instruction which cast type from user-written-type to VoidPtr in CodeGen.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131979/new/

https://reviews.llvm.org/D131979

Files:
  clang/include/clang/Basic/Builtins.def
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/catch-alignment-assumption-array.c
  clang/test/CodeGen/catch-alignment-assumption-ignorelist.c

Index: clang/test/CodeGen/catch-alignment-assumption-ignorelist.c
===
--- clang/test/CodeGen/catch-alignment-assumption-ignorelist.c
+++ clang/test/CodeGen/catch-alignment-assumption-ignorelist.c
@@ -26,3 +26,9 @@
 void *ignore_volatiles(volatile void * x) {
   return __builtin_assume_aligned(x, 1);
 }
+
+// CHECK-LABEL: ignore_array_volatiles
+void *ignore_array_volatiles() {
+  volatile int arr[] = {1};
+  return __builtin_assume_aligned(arr, 4);
+}
Index: clang/test/CodeGen/catch-alignment-assumption-array.c
===
--- /dev/null
+++ clang/test/CodeGen/catch-alignment-assumption-array.c
@@ -0,0 +1,32 @@
+// RUN: %clang_cc1 -no-opaque-pointers -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s
+// RUN: %clang_cc1 -no-opaque-pointers -fsanitize=alignment -fno-sanitize-recover=alignment -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s -implicit-check-not="call void @__ubsan_handle_alignment_assumption" --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-NORECOVER,CHECK-SANITIZE-UNREACHABLE
+// RUN: %clang_cc1 -no-opaque-pointers -fsanitize=alignment -fsanitize-recover=alignment -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s -implicit-check-not="call void @__ubsan_handle_alignment_assumption" --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-RECOVER
+// RUN: %clang_cc1 -no-opaque-pointers -fsanitize=alignment -fsanitize-trap=alignment -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s -implicit-check-not="call void @__ubsan_handle_alignment_assumption" --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-TRAP,CHECK-SANITIZE-UNREACHABLE
+
+// CHECK-SANITIZE-ANYRECOVER: @[[CHAR:.*]] = {{.*}} c"'char *'\00" }
+// CHECK-SANITIZE-ANYRECOVER: @[[ALIGNMENT_ASSUMPTION:.*]] = {{.*}}, i32 31, i32 35 }, {{.*}}* @[[CHAR]] }
+
+void *caller(void) {
+  char str[] = "";
+  // CHECK:   define{{.*}}
+  // CHECK-NEXT:  entry:
+  // CHECK-NEXT:%[[STR:.*]] = alloca [1 x i8], align 1
+  // CHECK-NEXT:%[[BITCAST:.*]] = bitcast [1 x i8]* %[[STR]] to i8*
+  // CHECK-NEXT:call void @llvm.memset.p0i8.i64(i8* align 1 %[[BITCAST]], i8 0, i64 1, i1 false)
+  // CHECK-NEXT:%[[ARRAYDECAY:.*]] = getelementptr inbounds [1 x i8], [1 x i8]* %[[STR]], i64 0, i64 0
+  // CHECK-SANITIZE-NEXT:   %[[PTRINT:.*]] = ptrtoint i8* %[[ARRAYDECAY]] to i64
+  // CHECK-SANITIZE-NEXT:   %[[MASKEDPTR:.*]] = and i64 %[[PTRINT]], 0
+  // CHECK-SANITIZE-NEXT:   %[[MASKCOND:.*]] = icmp eq i64 %[[MASKEDPTR]], 0
+  // CHECK-SANITIZE-NEXT:   %[[PTRINT_DUP:.*]] = ptrtoint i8* %[[ARRAYDECAY]] to i64, !nosanitize
+  // CHECK-SANITIZE-NEXT:   br i1 %[[MASKCOND]], label %[[CONT:.*]], label %[[HANDLER_ALIGNMENT_ASSUMPTION:[^,]+]],{{.*}} !nosanitize
+  // CHECK-SANITIZE:  [[HANDLER_ALIGNMENT_ASSUMPTION]]:
+  // CHECK-SANITIZE-NORECOVER-NEXT: call void @__ubsan_handle_alignment_assumption_abort(i8* bitcast ({ {{{.*}}}, {{{.*}}}, {{{.*}}}* }* @[[ALIGNMENT_ASSUMPTION]] to i8*), i64 %[[PTRINT_DUP]], i64 1, i64 0){{.*}}, !nosanitize
+  // CHECK-SANITIZE-RECOVER-NEXT:   call void @__ubsan_handle_alignment_assumption(i8* bitcast ({ {{{.*}}}, {{{.*}}}, {{{.*}}}* }* @[[ALIGNMENT_ASSUMPTION]] to i8*), i64 %[[PTRINT_DUP]], i64 1, i64 0){{.*}}, !nosanitize
+  // CHECK-SANITIZE-TRAP-NEXT:  call void @llvm.ubsantrap(i8 23){{.*}}, !nosanitize
+  // CHECK-SANITIZE-UNREACHABLE-NEXT:   unreachable, !nosanitize
+  // CHECK-SANITIZE:  [[CONT]]:
+  // 

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

In D131979#3753706 , @yihanaa wrote:

> Keep same behavior with current clang, don't emit an error when 1st arg is 
> volatile qualified.
>
> Ignore the implicit cast from user-written-type to 'const void *' on the 1st 
> arg, because CodeGen need user-written-type to generate correct 
> TypeDescriptor (this class in compiler-rt/UBSan), then, clang will emit cast 
> instruction which cast type from user-written-type to VoidPtr in CodeGen.
>
> But I still have some questions, for example: if someone pass a array 
> type(e.g. char[1]) as the 1st arg, what should the correct TypeDescriptor? 
> char[1] OR char *, clang will generate char * in this patch, you are experts 
> and what do you all think?

Hi John, can you please look at here, this is current version of patch's 
comment, and probably folded by Phabricator because of too many comments. 
Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131979/new/

https://reviews.llvm.org/D131979

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

In D131979#3756006 , @rjmccall wrote:

> In D131979#3753358 , @yihanaa wrote:
>
>> In D131979#3752208 , @rjmccall 
>> wrote:
>>
>>> From the test case, it looks like the builtin just ignores pointers to 
>>> volatile types, which should be preserved by the conversions you're now 
>>> doing in Sema.  That is, you should be able to just check 
>>> `Ptr->getType()->castAs()->getPointeeType().isVolatile()`.
>>>
>>> It would be different if it ignored pointers loaded out of volatile 
>>> l-values or something, but that's explicitly not what it's doing.
>>
>> Thanks for your tips John, form the test case global constants and 
>> compiler-rt ubsan class TypeDescriptor, it looks like pass an type 
>> description to __ubsan_handle_alignment_assumption, so, we have to use 
>> getSubExprAsWritten to get the origin(user written arg type) type 
>> description of 1st arg, but not 'const void *', for example,   in 
>> catch-alignment-assumption-builtin_assume_aligned-three-params.cpp, the 1st 
>> arg type description is 'char **', but if we don't use getSubExprAsWritten, 
>> all the type description will be 'const void *'
>
> The version of this patch which changed the builtin to use custom 
> type-checking didn't need to worry about this because doing that will stop 
> Sema from converting to `const void *`.  You seem to have taken all of that  
> code out and reverted to an earlier approach, though, and I don't see 
> anything in this review explaining why.
>
> Also, please don't ping for review on Monday when you submitted a new version 
> of the patch on Saturday.  Not only is that a very short period to be 
> demanding review during, but a lot of reviewers keep up with review as part 
> of their normal workday and cannot be expected to review over the weekend.

Thanks for your reply John, the patch of the previous version which will emit 
an error when 1st arg is volatile-qualified, we're  worried about the shift to 
emit an unconditional error on `volatile`. So, I'll try to separate that into 
its own patch and just fix the bug in this one. and I'm so sorry about have 
'pin' too often, this is due to time zone, I will pay attention to it later, 
and thanks for your tips!

In D131979#3752131 , @rjmccall wrote:

> I'm a little worried about the shift to emit an unconditional error on 
> `volatile`.  Can we at least separate that into its own patch and just fix 
> the bug in this one?  And please try to reach out whoever originally added 
> this feature to see if that restriction is okay vs. just ignoring it, because 
> it certainly looks like the `volatile` exception was intentionally designed.
>
> Other than that, this looks great, thank you.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131979/new/

https://reviews.llvm.org/D131979

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

pin～


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131979/new/

https://reviews.llvm.org/D131979

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

pin~


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131979/new/

https://reviews.llvm.org/D131979

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

[Wang Yihan via Phabricator via cfe-commits]

yihanaa updated this revision to Diff 456142.
yihanaa added a comment.

Format code and recovery builtin-functions.cpp(Wrongly modified in the last 
update)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131979/new/

https://reviews.llvm.org/D131979

Files:
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtin-assume-aligned.c
  clang/test/CodeGen/catch-alignment-assumption-array.c
  clang/test/CodeGen/catch-alignment-assumption-ignorelist.c

Index: clang/test/CodeGen/catch-alignment-assumption-ignorelist.c
===
--- clang/test/CodeGen/catch-alignment-assumption-ignorelist.c
+++ clang/test/CodeGen/catch-alignment-assumption-ignorelist.c
@@ -26,3 +26,9 @@
 void *ignore_volatiles(volatile void * x) {
   return __builtin_assume_aligned(x, 1);
 }
+
+// CHECK-LABEL: ignore_array_volatiles
+void *ignore_array_volatiles() {
+  volatile int arr[] = {1};
+  return __builtin_assume_aligned(arr, 4);
+}
Index: clang/test/CodeGen/catch-alignment-assumption-array.c
===
--- /dev/null
+++ clang/test/CodeGen/catch-alignment-assumption-array.c
@@ -0,0 +1,32 @@
+// RUN: %clang_cc1 -no-opaque-pointers -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s
+// RUN: %clang_cc1 -no-opaque-pointers -fsanitize=alignment -fno-sanitize-recover=alignment -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s -implicit-check-not="call void @__ubsan_handle_alignment_assumption" --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-NORECOVER,CHECK-SANITIZE-UNREACHABLE
+// RUN: %clang_cc1 -no-opaque-pointers -fsanitize=alignment -fsanitize-recover=alignment -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s -implicit-check-not="call void @__ubsan_handle_alignment_assumption" --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-RECOVER
+// RUN: %clang_cc1 -no-opaque-pointers -fsanitize=alignment -fsanitize-trap=alignment -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s -implicit-check-not="call void @__ubsan_handle_alignment_assumption" --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-TRAP,CHECK-SANITIZE-UNREACHABLE
+
+// CHECK-SANITIZE-ANYRECOVER: @[[CHAR:.*]] = {{.*}} c"'char *'\00" }
+// CHECK-SANITIZE-ANYRECOVER: @[[ALIGNMENT_ASSUMPTION:.*]] = {{.*}}, i32 31, i32 35 }, {{.*}}* @[[CHAR]] }
+
+void *caller(void) {
+  char str[] = "";
+  // CHECK:   define{{.*}}
+  // CHECK-NEXT:  entry:
+  // CHECK-NEXT:%[[STR:.*]] = alloca [1 x i8], align 1
+  // CHECK-NEXT:%[[BITCAST:.*]] = bitcast [1 x i8]* %[[STR]] to i8*
+  // CHECK-NEXT:call void @llvm.memset.p0i8.i64(i8* align 1 %[[BITCAST]], i8 0, i64 1, i1 false)
+  // CHECK-NEXT:%[[ARRAYDECAY:.*]] = getelementptr inbounds [1 x i8], [1 x i8]* %[[STR]], i64 0, i64 0
+  // CHECK-SANITIZE-NEXT:   %[[PTRINT:.*]] = ptrtoint i8* %[[ARRAYDECAY]] to i64
+  // CHECK-SANITIZE-NEXT:   %[[MASKEDPTR:.*]] = and i64 %[[PTRINT]], 0
+  // CHECK-SANITIZE-NEXT:   %[[MASKCOND:.*]] = icmp eq i64 %[[MASKEDPTR]], 0
+  // CHECK-SANITIZE-NEXT:   %[[PTRINT_DUP:.*]] = ptrtoint i8* %[[ARRAYDECAY]] to i64, !nosanitize
+  // CHECK-SANITIZE-NEXT:   br i1 %[[MASKCOND]], label %[[CONT:.*]], label %[[HANDLER_ALIGNMENT_ASSUMPTION:[^,]+]],{{.*}} !nosanitize
+  // CHECK-SANITIZE:  [[HANDLER_ALIGNMENT_ASSUMPTION]]:
+  // CHECK-SANITIZE-NORECOVER-NEXT: call void @__ubsan_handle_alignment_assumption_abort(i8* bitcast ({ {{{.*}}}, {{{.*}}}, {{{.*}}}* }* @[[ALIGNMENT_ASSUMPTION]] to i8*), i64 %[[PTRINT_DUP]], i64 1, i64 0){{.*}}, !nosanitize
+  // CHECK-SANITIZE-RECOVER-NEXT:   call void @__ubsan_handle_alignment_assumption(i8* bitcast ({ {{{.*}}}, {{{.*}}}, {{{.*}}}* }* @[[ALIGNMENT_ASSUMPTION]] to i8*), i64 %[[PTRINT_DUP]], i64 1, i64 0){{.*}}, !nosanitize
+  // CHECK-SANITIZE-TRAP-NEXT:  call void @llvm.ubsantrap(i8 23){{.*}}, !nosanitize
+  // CHECK-SANITIZE-UNREACHABLE-NEXT:   unreachable, !nosanitize
+  // CHECK-SANITIZE:  [[CONT]]:
+  // CHECK-NEXT:call void @llvm.assume(i1 true) [ "align"(i8* %[[ARRAYDECAY]], i64 1) ] 
+  // CHECK-NEXT:ret i8* %[[ARRAYDECAY]]
+  // CHECK-NEXT:  }
+  return __builtin_assume_aligned(str, 1);
+}
Index: clang/test/CodeGen/builtin-assume-aligned.c
===
--- clang/test/CodeGen/builtin-assume-aligned.c
+++ clang/test/CodeGen/builtin-assume-aligned.c
@@ -124,3 +124,11 @@
   a = __builtin_assume_aligned(a, 4294967296);
 return a[0];
 }
+
+// CHECK-LABEL: @test8()
+// CHECK-NEXT:  entry:
+// 

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

[Wang Yihan via Phabricator via cfe-commits]

yihanaa updated this revision to Diff 456140.
yihanaa added a comment.

Keep same behavior with current clang, don't emit an error when 1st arg is 
volatile qualified.

Ignore the implicit cast from user-written-type to 'const void *' on the 1st 
arg, because CodeGen need user-written-type to generate correct TypeDescriptor 
(this class in compiler-rt/UBSan), then, clang will emit cast instruction which 
cast type from user-written-type to VoidPtr in CodeGen.

But I still have some questions, for example: if someone pass a array type(e.g. 
char[1]) as the 1st arg, what should the correct TypeDescriptor? char[1] OR 
char *, clang will generate char * in this patch, you are experts and what do 
you all think?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131979/new/

https://reviews.llvm.org/D131979

Files:
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Analysis/builtin-functions.cpp
  clang/test/CodeGen/builtin-assume-aligned.c
  clang/test/CodeGen/catch-alignment-assumption-array.c
  clang/test/CodeGen/catch-alignment-assumption-ignorelist.c

Index: clang/test/CodeGen/catch-alignment-assumption-ignorelist.c
===
--- clang/test/CodeGen/catch-alignment-assumption-ignorelist.c
+++ clang/test/CodeGen/catch-alignment-assumption-ignorelist.c
@@ -26,3 +26,9 @@
 void *ignore_volatiles(volatile void * x) {
   return __builtin_assume_aligned(x, 1);
 }
+
+// CHECK-LABEL: ignore_array_volatiles
+void *ignore_array_volatiles() {
+  volatile int arr[] = {1};
+  return __builtin_assume_aligned(arr, 4);
+}
Index: clang/test/CodeGen/catch-alignment-assumption-array.c
===
--- /dev/null
+++ clang/test/CodeGen/catch-alignment-assumption-array.c
@@ -0,0 +1,32 @@
+// RUN: %clang_cc1 -no-opaque-pointers -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s
+// RUN: %clang_cc1 -no-opaque-pointers -fsanitize=alignment -fno-sanitize-recover=alignment -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s -implicit-check-not="call void @__ubsan_handle_alignment_assumption" --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-NORECOVER,CHECK-SANITIZE-UNREACHABLE
+// RUN: %clang_cc1 -no-opaque-pointers -fsanitize=alignment -fsanitize-recover=alignment -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s -implicit-check-not="call void @__ubsan_handle_alignment_assumption" --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-RECOVER
+// RUN: %clang_cc1 -no-opaque-pointers -fsanitize=alignment -fsanitize-trap=alignment -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s -implicit-check-not="call void @__ubsan_handle_alignment_assumption" --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-TRAP,CHECK-SANITIZE-UNREACHABLE
+
+// CHECK-SANITIZE-ANYRECOVER: @[[CHAR:.*]] = {{.*}} c"'char *'\00" }
+// CHECK-SANITIZE-ANYRECOVER: @[[ALIGNMENT_ASSUMPTION:.*]] = {{.*}}, i32 31, i32 35 }, {{.*}}* @[[CHAR]] }
+
+void *caller(void) {
+  char str[] = "";
+  // CHECK:   define{{.*}}
+  // CHECK-NEXT:  entry:
+  // CHECK-NEXT:%[[STR:.*]] = alloca [1 x i8], align 1
+  // CHECK-NEXT:%[[BITCAST:.*]] = bitcast [1 x i8]* %[[STR]] to i8*
+  // CHECK-NEXT:call void @llvm.memset.p0i8.i64(i8* align 1 %[[BITCAST]], i8 0, i64 1, i1 false)
+  // CHECK-NEXT:%[[ARRAYDECAY:.*]] = getelementptr inbounds [1 x i8], [1 x i8]* %[[STR]], i64 0, i64 0
+  // CHECK-SANITIZE-NEXT:   %[[PTRINT:.*]] = ptrtoint i8* %[[ARRAYDECAY]] to i64
+  // CHECK-SANITIZE-NEXT:   %[[MASKEDPTR:.*]] = and i64 %[[PTRINT]], 0
+  // CHECK-SANITIZE-NEXT:   %[[MASKCOND:.*]] = icmp eq i64 %[[MASKEDPTR]], 0
+  // CHECK-SANITIZE-NEXT:   %[[PTRINT_DUP:.*]] = ptrtoint i8* %[[ARRAYDECAY]] to i64, !nosanitize
+  // CHECK-SANITIZE-NEXT:   br i1 %[[MASKCOND]], label %[[CONT:.*]], label %[[HANDLER_ALIGNMENT_ASSUMPTION:[^,]+]],{{.*}} !nosanitize
+  // CHECK-SANITIZE:  [[HANDLER_ALIGNMENT_ASSUMPTION]]:
+  // CHECK-SANITIZE-NORECOVER-NEXT: call void @__ubsan_handle_alignment_assumption_abort(i8* bitcast ({ {{{.*}}}, {{{.*}}}, {{{.*}}}* }* @[[ALIGNMENT_ASSUMPTION]] to i8*), i64 %[[PTRINT_DUP]], i64 1, i64 0){{.*}}, !nosanitize
+  // CHECK-SANITIZE-RECOVER-NEXT:   call void @__ubsan_handle_alignment_assumption(i8* bitcast ({ {{{.*}}}, {{{.*}}}, {{{.*}}}* }* @[[ALIGNMENT_ASSUMPTION]] to i8*), i64 %[[PTRINT_DUP]], i64 1, i64 0){{.*}}, !nosanitize
+  // CHECK-SANITIZE-TRAP-NEXT:  call void @llvm.ubsantrap(i8 23){{.*}}, !nosanitize
+  // CHECK-SANITIZE-UNREACHABLE-NEXT:   unreachable, !nosanitize
+  // CHECK-SANITIZE:  [[CONT]]:
+  // CHECK-NEXT: 

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

In D131979#3752208 , @rjmccall wrote:

> From the test case, it looks like the builtin just ignores pointers to 
> volatile types, which should be preserved by the conversions you're now doing 
> in Sema.  That is, you should be able to just check 
> `Ptr->getType()->castAs()->getPointeeType().isVolatile()`.
>
> It would be different if it ignored pointers loaded out of volatile l-values 
> or something, but that's explicitly not what it's doing.

Thanks for your tips John, form the test case global constants and compiler-rt 
ubsan class TypeDescriptor, it looks like pass an type description to 
__ubsan_handle_alignment_assumption, so, we have to use getSubExprAsWritten to 
get the origin(user written arg type) type description of 1st arg, but not 
'const void *', for example,   in 
catch-alignment-assumption-builtin_assume_aligned-three-params.cpp, the 1st arg 
type description is 'char **', but if we don't use getSubExprAsWritten, all the 
type description will be 'const void *'


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131979/new/

https://reviews.llvm.org/D131979

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

In D131979#3752131 , @rjmccall wrote:

> I'm a little worried about the shift to emit an unconditional error on 
> `volatile`.  Can we at least separate that into its own patch and just fix 
> the bug in this one?  And please try to reach out whoever originally added 
> this feature to see if that restriction is okay vs. just ignoring it, because 
> it certainly looks like the `volatile` exception was intentionally designed.
>
> Other than that, this looks great, thank you.

Thanks for your review, John,I agree. If we just fix this crash, I didn't think 
of a good way to remove the call to getSubExprAsWritten in CodeGen. But I'll 
try to fix crash first.

Maybe there should be a discussion about emit an error for volatile in the 
community.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131979/new/

https://reviews.llvm.org/D131979

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

[Wang Yihan via Phabricator via cfe-commits]

yihanaa updated this revision to Diff 455918.
yihanaa added a comment.

Thanks John and Erich for your comments.
In this update:

Check args in Sema, and both in C and C++, emit an error if the 1st arg of 
__builtin_assume_aligned is volatile qualified or 1st arg not a pointer type.

Remove getSubExprAsWritten in emitAlignmentAssumption, and Emit a cast if 1st 
arg not void * type.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131979/new/

https://reviews.llvm.org/D131979

Files:
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/Sema/SemaChecking.cpp
  
clang/test/CodeGen/catch-alignment-assumption-builtin_assume_aligned-first-arg-cannot-volatile.c
  
clang/test/CodeGen/catch-alignment-assumption-builtin_assume_aligned-first-arg-not-pointer.c
  
clang/test/CodeGen/catch-alignment-assumption-builtin_assume_aligned-implicit-cast.c
  clang/test/CodeGen/catch-alignment-assumption-ignorelist.c

Index: clang/test/CodeGen/catch-alignment-assumption-ignorelist.c
===
--- clang/test/CodeGen/catch-alignment-assumption-ignorelist.c
+++ clang/test/CodeGen/catch-alignment-assumption-ignorelist.c
@@ -21,8 +21,3 @@
   // CHECK: call void @__ubsan_handle_alignment_assumption(
   return __builtin_assume_aligned(x, 1);
 }
-
-// CHECK-LABEL: ignore_volatiles
-void *ignore_volatiles(volatile void * x) {
-  return __builtin_assume_aligned(x, 1);
-}
Index: clang/test/CodeGen/catch-alignment-assumption-builtin_assume_aligned-implicit-cast.c
===
--- /dev/null
+++ clang/test/CodeGen/catch-alignment-assumption-builtin_assume_aligned-implicit-cast.c
@@ -0,0 +1,32 @@
+// RUN: %clang_cc1 -no-opaque-pointers -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s
+// RUN: %clang_cc1 -no-opaque-pointers -fsanitize=alignment -fno-sanitize-recover=alignment -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s -implicit-check-not="call void @__ubsan_handle_alignment_assumption" --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-NORECOVER,CHECK-SANITIZE-UNREACHABLE
+// RUN: %clang_cc1 -no-opaque-pointers -fsanitize=alignment -fsanitize-recover=alignment -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s -implicit-check-not="call void @__ubsan_handle_alignment_assumption" --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-RECOVER
+// RUN: %clang_cc1 -no-opaque-pointers -fsanitize=alignment -fsanitize-trap=alignment -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s -implicit-check-not="call void @__ubsan_handle_alignment_assumption" --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-TRAP,CHECK-SANITIZE-UNREACHABLE
+
+// CHECK-SANITIZE-ANYRECOVER: @[[CHAR:.*]] = {{.*}} c"'char *'\00" }
+// CHECK-SANITIZE-ANYRECOVER: @[[ALIGNMENT_ASSUMPTION:.*]] = {{.*}}, i32 31, i32 35 }, {{.*}}* @[[CHAR]] }
+
+void *caller(void) {
+  char str[] = "";
+  // CHECK:   define{{.*}}
+  // CHECK-NEXT:  entry:
+  // CHECK-NEXT:%[[STR:.*]] = alloca [1 x i8], align 1
+  // CHECK-NEXT:%[[BITCAST:.*]] = bitcast [1 x i8]* %[[STR]] to i8*
+  // CHECK-NEXT:call void @llvm.memset.p0i8.i64(i8* align 1 %[[BITCAST]], i8 0, i64 1, i1 false)
+  // CHECK-NEXT:%[[ARRAYDECAY:.*]] = getelementptr inbounds [1 x i8], [1 x i8]* %[[STR]], i64 0, i64 0
+  // CHECK-SANITIZE-NEXT:   %[[PTRINT:.*]] = ptrtoint i8* %[[ARRAYDECAY]] to i64
+  // CHECK-SANITIZE-NEXT:   %[[MASKEDPTR:.*]] = and i64 %[[PTRINT]], 0
+  // CHECK-SANITIZE-NEXT:   %[[MASKCOND:.*]] = icmp eq i64 %[[MASKEDPTR]], 0
+  // CHECK-SANITIZE-NEXT:   %[[PTRINT_DUP:.*]] = ptrtoint i8* %[[ARRAYDECAY]] to i64, !nosanitize
+  // CHECK-SANITIZE-NEXT:   br i1 %[[MASKCOND]], label %[[CONT:.*]], label %[[HANDLER_ALIGNMENT_ASSUMPTION:[^,]+]],{{.*}} !nosanitize
+  // CHECK-SANITIZE:  [[HANDLER_ALIGNMENT_ASSUMPTION]]:
+  // CHECK-SANITIZE-NORECOVER-NEXT: call void @__ubsan_handle_alignment_assumption_abort(i8* bitcast ({ {{{.*}}}, {{{.*}}}, {{{.*}}}* }* @[[ALIGNMENT_ASSUMPTION]] to i8*), i64 %[[PTRINT_DUP]], i64 1, i64 0){{.*}}, !nosanitize
+  // CHECK-SANITIZE-RECOVER-NEXT:   call void @__ubsan_handle_alignment_assumption(i8* bitcast ({ {{{.*}}}, {{{.*}}}, {{{.*}}}* }* @[[ALIGNMENT_ASSUMPTION]] to i8*), i64 %[[PTRINT_DUP]], i64 1, i64 0){{.*}}, !nosanitize
+  // CHECK-SANITIZE-TRAP-NEXT:  call void @llvm.ubsantrap(i8 23){{.*}}, !nosanitize
+  // CHECK-SANITIZE-UNREACHABLE-NEXT:   unreachable, !nosanitize
+  // CHECK-SANITIZE:  [[CONT]]:
+  // CHECK-NEXT:call void @llvm.assume(i1 true) [ 

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

In D131979#3743127 , @rjmccall wrote:

> In D131979#3742399 , @yihanaa wrote:
>
>> As far as I know, turning on the -fsanitizer=alignment options when calling 
>> __builtin_assume_aligned in C code, if
>> the 1st arg has volatile qualifier, Clang will emit "call void 
>> @__ubsan_handle_alignment_assumption(...)" in CodeGen,
>> else Clang will emit an warning and ignore "call void 
>> @__ubsan_handle_alignment_assumption(...)". But the same situation in C++, 
>> Clang will generate an error in Sema.
>
> Honestly, that sounds like bad behavior, exactly what you'd get if Sema 
> naively tried to force a conversion to some specific pointer type like 
> `void*` instead of converting it to an r-value and then requiring it to have 
> pointer type.  This is a fairly common bug for builtins that behave as if 
> they were overloaded; the fix is to give the builtin custom type-checking so 
> that it will work correctly with unusual qualifiers like address spaces and 
> `restrict`.  I can walk you through that, but you can start by looking at 
> what `SemaBuiltinAtomicOverloaded` does for its pointer argument.
>
> If we want IRGen to not emit a check for pointers to `volatile`, that's an 
> easy check to do in IRGen if we set the AST up properly in the first place.
>
> John.
>
>> So I think, in order to keep this patch consistent with recent version of 
>> Clang and GCC behavior, when compile C code, Clang
>> should not directly emit an error and exit in Sema, but should check the 1st 
>> arg's volatile qualifier in CodeGen and decide 
>> whether to emit "call void @__ubsan_handle_alignment_assumption(...)".
>>
>> But, I agree to use other ways to replace use getSubExprAsWritten() in 
>> CodeGen to check the 1st arg 'isVolatile', what do you all think about?
>>
>> For more information about UBSan, see 
>> https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html and search 
>> 'volatile' on this website.
>>
>> "The null, alignment, object-size, local-bounds, and vptr checks do not 
>> apply to pointers to types with the volatile qualifier."
>> C https://godbolt.org/z/xv35fG14r
>> C++ https://godbolt.org/z/qfje6sErE
>
> Honestly,

I'm sorry I couldn't respond sooner.
Thanks for your reply john, I agree with you, and I'd like to try it if we 
check args in Sema.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131979/new/

https://reviews.llvm.org/D131979

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

As far as I know, turning on the -fsanitizer=alignment options when calling 
__builtin_assume_aligned in C code, if
the 1st arg has volatile qualifier, Clang will emit "call void 
@__ubsan_handle_alignment_assumption(...)" in CodeGen,
else Clang will emit an warning and ignore "call void 
@__ubsan_handle_alignment_assumption(...)". But the same situation in C++, 
Clang will generate an error in Sema.

So I think, in order to keep this patch consistent with recent version of Clang 
and GCC behavior, when compile C code, Clang
should not directly emit an error and exit in Sema, but should check the 1st 
arg's volatile qualifier in CodeGen and decide 
whether to emit "call void @__ubsan_handle_alignment_assumption(...)".

But, I agree to use other ways to replace use getSubExprAsWritten() in CodeGen 
to check the 1st arg 'isVolatile', what do you all think about?

For more information about UBSan, see 
https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html and search 
'volatile' on this website.

"The null, alignment, object-size, local-bounds, and vptr checks do not apply 
to pointers to types with the volatile qualifier."
C https://godbolt.org/z/xv35fG14r
C++ https://godbolt.org/z/qfje6sErE


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131979/new/

https://reviews.llvm.org/D131979

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

Thanks for your suggestion @erichkeane @rjmccall , I will try to do that


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131979/new/

https://reviews.llvm.org/D131979

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

In D131979#3726686 , @erichkeane 
wrote:

> I think the correct answer here is instead to prohibit using this on 
> array/string literal types in Sema instead of just ignoring the call.

Thanks for you replay @erichkeane , in 
clang/lib/CodeGen/CodeGenFunction.cpp:2442, Since E = CE->getSubExprAsWritten() 
is used, implicit cast is no longer visible in  
clang::CodeGenCodeGenFunction::emitAlignmentAssumptionCheck

  


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131979/new/

https://reviews.llvm.org/D131979

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

[Wang Yihan via Phabricator via cfe-commits]

yihanaa created this revision.
yihanaa added reviewers: rjmccall, aaron.ballman, erichkeane, lebedev.ri.
yihanaa added a project: clang.
Herald added a project: All.
yihanaa requested review of this revision.
Herald added a subscriber: cfe-commits.

Clang will crash when __builtin_assume_aligned's 1st arg is array type(or 
string literal).
Open issue: https://github.com/llvm/llvm-project/issues/57169


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131979

Files:
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/test/CodeGen/builtin-assume-aligned.c
  clang/test/CodeGen/catch-alignment-assumption-ignorelist.c


Index: clang/test/CodeGen/catch-alignment-assumption-ignorelist.c
===
--- clang/test/CodeGen/catch-alignment-assumption-ignorelist.c
+++ clang/test/CodeGen/catch-alignment-assumption-ignorelist.c
@@ -26,3 +26,9 @@
 void *ignore_volatiles(volatile void * x) {
   return __builtin_assume_aligned(x, 1);
 }
+
+// CHECK-LABEL: ignore_volatiles
+void ignore_volatiles_array() {
+  volatile char arr[] = "a";
+  (void)__builtin_assume_aligned(arr, 1);
+}
Index: clang/test/CodeGen/builtin-assume-aligned.c
===
--- clang/test/CodeGen/builtin-assume-aligned.c
+++ clang/test/CodeGen/builtin-assume-aligned.c
@@ -1,6 +1,8 @@
 // NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
 // RUN: %clang_cc1 -no-opaque-pointers -triple x86_64-unknown-unknown 
-emit-llvm -o - %s | FileCheck %s
 
+// CHECK: [[TEST7_STR:@.*]] = private unnamed_addr constant [2 x i8] c"a\00", 
align 1
+
 // CHECK-LABEL: @test1(
 // CHECK-NEXT:  entry:
 // CHECK-NEXT:[[A_ADDR:%.*]] = alloca i32*, align 8
@@ -124,3 +126,10 @@
   a = __builtin_assume_aligned(a, 4294967296);
 return a[0];
 }
+
+// CHECK-LABEL: @test7(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:call void @llvm.assume(i1 true) [ "align"(i8* getelementptr 
inbounds ([2 x i8], [2 x i8]* [[TEST7_STR]], i64 0, i64 0), i64 1) ]
+void test7(void) {
+  (void) __builtin_assume_aligned("a", 1);
+}
Index: clang/lib/CodeGen/CodeGenFunction.cpp
===
--- clang/lib/CodeGen/CodeGenFunction.cpp
+++ clang/lib/CodeGen/CodeGenFunction.cpp
@@ -2711,8 +2711,14 @@
 
   // Don't check pointers to volatile data. The behavior here is 
implementation-
   // defined.
-  if (Ty->getPointeeType().isVolatileQualified())
-return;
+  if (Ty->isPointerType()) {
+if (Ty->getPointeeType().isVolatileQualified())
+  return;
+  } else {
+// Ty maybe an array type
+if (Ty.isVolatileQualified())
+  return;
+  }
 
   // We need to temorairly remove the assumption so we can insert the
   // sanitizer check before it, else the check will be dropped by 
optimizations.


Index: clang/test/CodeGen/catch-alignment-assumption-ignorelist.c
===
--- clang/test/CodeGen/catch-alignment-assumption-ignorelist.c
+++ clang/test/CodeGen/catch-alignment-assumption-ignorelist.c
@@ -26,3 +26,9 @@
 void *ignore_volatiles(volatile void * x) {
   return __builtin_assume_aligned(x, 1);
 }
+
+// CHECK-LABEL: ignore_volatiles
+void ignore_volatiles_array() {
+  volatile char arr[] = "a";
+  (void)__builtin_assume_aligned(arr, 1);
+}
Index: clang/test/CodeGen/builtin-assume-aligned.c
===
--- clang/test/CodeGen/builtin-assume-aligned.c
+++ clang/test/CodeGen/builtin-assume-aligned.c
@@ -1,6 +1,8 @@
 // NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
 // RUN: %clang_cc1 -no-opaque-pointers -triple x86_64-unknown-unknown -emit-llvm -o - %s | FileCheck %s
 
+// CHECK: [[TEST7_STR:@.*]] = private unnamed_addr constant [2 x i8] c"a\00", align 1
+
 // CHECK-LABEL: @test1(
 // CHECK-NEXT:  entry:
 // CHECK-NEXT:[[A_ADDR:%.*]] = alloca i32*, align 8
@@ -124,3 +126,10 @@
   a = __builtin_assume_aligned(a, 4294967296);
 return a[0];
 }
+
+// CHECK-LABEL: @test7(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:call void @llvm.assume(i1 true) [ "align"(i8* getelementptr inbounds ([2 x i8], [2 x i8]* [[TEST7_STR]], i64 0, i64 0), i64 1) ]
+void test7(void) {
+  (void) __builtin_assume_aligned("a", 1);
+}
Index: clang/lib/CodeGen/CodeGenFunction.cpp
===
--- clang/lib/CodeGen/CodeGenFunction.cpp
+++ clang/lib/CodeGen/CodeGenFunction.cpp
@@ -2711,8 +2711,14 @@
 
   // Don't check pointers to volatile data. The behavior here is implementation-
   // defined.
-  if (Ty->getPointeeType().isVolatileQualified())
-return;
+  if (Ty->isPointerType()) {
+if (Ty->getPointeeType().isVolatileQualified())
+  return;
+  } else {
+// Ty maybe an array type
+if (Ty.isVolatileQualified())
+  return;
+  }
 
   // We need to temorairly remove the assumption so we can insert the
   // sanitizer 

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

This is unfortunately true, unless we generate some code that calculates the 
length of the string at runtime. I find it difficult to draw the line between 
defining this built-in functionality and developers developing printf-like 
functionality, if built-in does too much, flexibility will decrease, if 
built-in does less, (features like length limits) then develop personnel may 
need to do some work.   o(≧口≦)o


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124221/new/

https://reviews.llvm.org/D124221

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

I found LLDB have this feature. LLDB uses "..." to indicate that only part of 
the string is now printed, and it also have a command to modify length limit. 
This approach is similar to the idea of  @erichkeane,  can we do the same？what 
do you all think?

  typedef signed char int8_t;
  typedef unsigned char uint8_t;
  
  int printf(const char *, ...);
  
  int main() {
struct U20A {
  const char *str;
};
  
struct U20A a = {
.str = 
""
};
  __builtin_dump_struct(, );
  }



  (lldb) p a.str
  (const char *) $1 = 0x0001329a 
""...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:433
+
+  llvm::StringRef getFormatSpecifier(QualType T) {
+if (auto *BT = T->getAs()) {

yihanaa wrote:
> erichkeane wrote:
> > rsmith wrote:
> > > yihanaa wrote:
> > > > rsmith wrote:
> > > > > rsmith wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > yihanaa wrote:
> > > > > > > > I think this is better maintained in 
> > > > > > > > "clang/AST/FormatString.h". For example 
> > > > > > > > analyze_printf::PrintfSpecifier can get format specifier, what 
> > > > > > > > do you all think about?
> > > > > > > +1 to this suggestion -- my hope is that we could generalize it 
> > > > > > > more then as I notice there are missing specifiers for things 
> > > > > > > like intmax_t, size_t, ptrdiff_t, _Decimal types, etc. Plus, that 
> > > > > > > will hopefully make it easier for __builtin_dump_struct to 
> > > > > > > benefit when new format specifiers are added, such as ones for 
> > > > > > > printing a _BitInt.
> > > > > > I am somewhat uncertain: every one of these is making arbitrary 
> > > > > > choices about how to format the value, so it's not clear to me that 
> > > > > > this is general logic rather than something specific to 
> > > > > > `__builtin_dump_struct`. For example, using `%f` rather than one of 
> > > > > > the myriad other ways of formatting `double` is somewhat arbitrary. 
> > > > > > Using `%s` for any `const char*` is *extremely* arbitrary and will 
> > > > > > be wrong and will cause crashes in some cases, but it may be the 
> > > > > > pragmatically correct choice for a dumping utility. A 
> > > > > > general-purpose mechanism would use `%p` for all kinds of pointer.
> > > > > > 
> > > > > > We could perhaps factor out the formatting for cases where there is 
> > > > > > a clear canonical default formatting, such as for integer types and 
> > > > > > probably `%p` for pointers, then call that from here with some 
> > > > > > special-casing, but without a second consumer for that 
> > > > > > functionality it's really not clear to me what form it should take.
> > > > > I went ahead and did this, mostly to match concurrent changes to the 
> > > > > old implementation. There are a few cases where our existing "guess a 
> > > > > format specifier" logic does the wrong thing for dumping purposes, 
> > > > > which I've explicitly handled -- things like wanting to dump a `char` 
> > > > > / `signed char` / `unsigned char` member as a number rather than as a 
> > > > > (potentially non-printable or whitespace) character.
> > > >  When I was patching that old implementation, I found that for uint8_t, 
> > > > int8_t, Clang's existing "guess a format specifier" logic would treat 
> > > > the value as an integer, but for unsigned char, signed char, char 
> > > > types, it would Treat it as a character, please look at this example ( 
> > > > https://godbolt.org/z/ooqn4468T ), I guess this existing logic may have 
> > > > made some special judgment.
> > > Yeah. I think in the case where we see some random call to `printf`, `%c` 
> > > is probably the right thing to guess here, but it doesn't seem 
> > > appropriate to me to use this in a dumping routine. If we could dump as 
> > > `'x'` for printable characters and as `'\xAB'` for everything else, 
> > > that'd be great, but `printf` can't do that itself and I'm not sure we 
> > > want to be injecting calls to `isprint` or whatever to make that work. 
> > > Dumping as an integer always seems like it's probably the least-bad 
> > > option.
> > > 
> > > Somewhat related: I wonder if we should use `"\"%s\""` instead of simply 
> > > `"%s"` when dumping a `const char*`. That's not ideal but probably 
> > > clearer than the current dump output.
> > I see value to having strings with SOME level of delimiter, if at least to 
> > handle cases when the string itself has a newline in it.
> This looks good, but the string may need to be escaped (if there are some 
> special characters in the original string)
Maybe users should wrap the printf function, and then pass the wrapper function 
as a parameter, so that they can do some custom things


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124221/new/

https://reviews.llvm.org/D124221

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:433
+
+  llvm::StringRef getFormatSpecifier(QualType T) {
+if (auto *BT = T->getAs()) {

erichkeane wrote:
> rsmith wrote:
> > yihanaa wrote:
> > > rsmith wrote:
> > > > rsmith wrote:
> > > > > aaron.ballman wrote:
> > > > > > yihanaa wrote:
> > > > > > > I think this is better maintained in "clang/AST/FormatString.h". 
> > > > > > > For example analyze_printf::PrintfSpecifier can get format 
> > > > > > > specifier, what do you all think about?
> > > > > > +1 to this suggestion -- my hope is that we could generalize it 
> > > > > > more then as I notice there are missing specifiers for things like 
> > > > > > intmax_t, size_t, ptrdiff_t, _Decimal types, etc. Plus, that will 
> > > > > > hopefully make it easier for __builtin_dump_struct to benefit when 
> > > > > > new format specifiers are added, such as ones for printing a 
> > > > > > _BitInt.
> > > > > I am somewhat uncertain: every one of these is making arbitrary 
> > > > > choices about how to format the value, so it's not clear to me that 
> > > > > this is general logic rather than something specific to 
> > > > > `__builtin_dump_struct`. For example, using `%f` rather than one of 
> > > > > the myriad other ways of formatting `double` is somewhat arbitrary. 
> > > > > Using `%s` for any `const char*` is *extremely* arbitrary and will be 
> > > > > wrong and will cause crashes in some cases, but it may be the 
> > > > > pragmatically correct choice for a dumping utility. A general-purpose 
> > > > > mechanism would use `%p` for all kinds of pointer.
> > > > > 
> > > > > We could perhaps factor out the formatting for cases where there is a 
> > > > > clear canonical default formatting, such as for integer types and 
> > > > > probably `%p` for pointers, then call that from here with some 
> > > > > special-casing, but without a second consumer for that functionality 
> > > > > it's really not clear to me what form it should take.
> > > > I went ahead and did this, mostly to match concurrent changes to the 
> > > > old implementation. There are a few cases where our existing "guess a 
> > > > format specifier" logic does the wrong thing for dumping purposes, 
> > > > which I've explicitly handled -- things like wanting to dump a `char` / 
> > > > `signed char` / `unsigned char` member as a number rather than as a 
> > > > (potentially non-printable or whitespace) character.
> > >  When I was patching that old implementation, I found that for uint8_t, 
> > > int8_t, Clang's existing "guess a format specifier" logic would treat the 
> > > value as an integer, but for unsigned char, signed char, char types, it 
> > > would Treat it as a character, please look at this example ( 
> > > https://godbolt.org/z/ooqn4468T ), I guess this existing logic may have 
> > > made some special judgment.
> > Yeah. I think in the case where we see some random call to `printf`, `%c` 
> > is probably the right thing to guess here, but it doesn't seem appropriate 
> > to me to use this in a dumping routine. If we could dump as `'x'` for 
> > printable characters and as `'\xAB'` for everything else, that'd be great, 
> > but `printf` can't do that itself and I'm not sure we want to be injecting 
> > calls to `isprint` or whatever to make that work. Dumping as an integer 
> > always seems like it's probably the least-bad option.
> > 
> > Somewhat related: I wonder if we should use `"\"%s\""` instead of simply 
> > `"%s"` when dumping a `const char*`. That's not ideal but probably clearer 
> > than the current dump output.
> I see value to having strings with SOME level of delimiter, if at least to 
> handle cases when the string itself has a newline in it.
This looks good, but the string may need to be escaped (if there are some 
special characters in the original string)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124221/new/

https://reviews.llvm.org/D124221

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:433
+
+  llvm::StringRef getFormatSpecifier(QualType T) {
+if (auto *BT = T->getAs()) {

rsmith wrote:
> rsmith wrote:
> > aaron.ballman wrote:
> > > yihanaa wrote:
> > > > I think this is better maintained in "clang/AST/FormatString.h". For 
> > > > example analyze_printf::PrintfSpecifier can get format specifier, what 
> > > > do you all think about?
> > > +1 to this suggestion -- my hope is that we could generalize it more then 
> > > as I notice there are missing specifiers for things like intmax_t, 
> > > size_t, ptrdiff_t, _Decimal types, etc. Plus, that will hopefully make it 
> > > easier for __builtin_dump_struct to benefit when new format specifiers 
> > > are added, such as ones for printing a _BitInt.
> > I am somewhat uncertain: every one of these is making arbitrary choices 
> > about how to format the value, so it's not clear to me that this is general 
> > logic rather than something specific to `__builtin_dump_struct`. For 
> > example, using `%f` rather than one of the myriad other ways of formatting 
> > `double` is somewhat arbitrary. Using `%s` for any `const char*` is 
> > *extremely* arbitrary and will be wrong and will cause crashes in some 
> > cases, but it may be the pragmatically correct choice for a dumping 
> > utility. A general-purpose mechanism would use `%p` for all kinds of 
> > pointer.
> > 
> > We could perhaps factor out the formatting for cases where there is a clear 
> > canonical default formatting, such as for integer types and probably `%p` 
> > for pointers, then call that from here with some special-casing, but 
> > without a second consumer for that functionality it's really not clear to 
> > me what form it should take.
> I went ahead and did this, mostly to match concurrent changes to the old 
> implementation. There are a few cases where our existing "guess a format 
> specifier" logic does the wrong thing for dumping purposes, which I've 
> explicitly handled -- things like wanting to dump a `char` / `signed char` / 
> `unsigned char` member as a number rather than as a (potentially 
> non-printable or whitespace) character.
 When I was patching that old implementation, I found that for uint8_t, int8_t, 
Clang's existing "guess a format specifier" logic would treat the value as an 
integer, but for unsigned char, signed char, char types, it would Treat it as a 
character, please look at this example ( https://godbolt.org/z/ooqn4468T ), I 
guess this existing logic may have made some special judgment.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124221/new/

https://reviews.llvm.org/D124221

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

Do we need to do argument promotion here，ahh...maybe i understood wrong


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124221/new/

https://reviews.llvm.org/D124221

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:573
+  // We don't know how to print this field. Print out its address
+  // with a format specifier that a smart tool will be able to
+  // recognize and treat specially.

Can we generate a warning diagnostic message when we see an unsupported type 
(perhaps temporarily not supported), what do you all think about?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124221/new/

https://reviews.llvm.org/D124221

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

This implementation looks good to me


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124221/new/

https://reviews.llvm.org/D124221

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:174
   - Improve the dump format, dump both bitwidth(if its a bitfield) and field 
value.
-  - Remove anonymous tag locations.
-  - Beautify dump format, add indent for nested struct and struct members.
+  - Remove anonymous tag locations and flatten anonymous struct members.
+  - Beautify dump format, add indent for struct members.

I tried it, D124221 will still generate anonymous tag locations, this place 
needs to be corrected, or removed anonymous tag locations.

int main() {
  struct U20A {
_Bool a;
unsigned : 32;
struct {
  int x;
  int y;
} c;
  };

  struct U20A a = {
  .a = 1,
  .c.x = 1998
  };
__builtin_dump_struct(, printf);
}

struct U20A {
_Bool a = 1
struct (unnamed struct at ./builtin_dump_struct.c:10:5) c = {
int x = 1998
int y = 0
}
}


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124221/new/

https://reviews.llvm.org/D124221

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:427
+llvm::SmallString<32> Indent;
+Indent.resize(Depth * 4, ' ');
+return getStringLiteral(Indent);

What do you think of PrintingPolicy.Indentation here?



Comment at: clang/lib/Sema/SemaChecking.cpp:433
+
+  llvm::StringRef getFormatSpecifier(QualType T) {
+if (auto *BT = T->getAs()) {

I think this is better maintained in "clang/AST/FormatString.h". For example 
analyze_printf::PrintfSpecifier can get format specifier, what do you all think 
about?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124221/new/

https://reviews.llvm.org/D124221

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122920: [Clang][CodeGen]Fix __builtin_dump_struct missing record type field name

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

Thank you all commit in eaca933c59fd61b3df4697b5fae0eeec67acfaeb 



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122920/new/

https://reviews.llvm.org/D122920

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122920: [Clang][CodeGen]Fix __builtin_dump_struct missing record type field name

[Wang Yihan via Phabricator via cfe-commits]

yihanaa marked an inline comment as done.
yihanaa added a comment.

Thanks for review @erichkeane and @aaron.ballman , I have add a newline back to 
the end of the test, and waiting for ci


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122920/new/

https://reviews.llvm.org/D122920

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122920: [Clang][CodeGen]Fix __builtin_dump_struct missing record type field name

[Wang Yihan via Phabricator via cfe-commits]

yihanaa updated this revision to Diff 425861.
yihanaa added a comment.

Add the newline back to the end of the test.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122920/new/

https://reviews.llvm.org/D122920

Files:
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/test/CodeGen/dump-struct-builtin.c

Index: clang/test/CodeGen/dump-struct-builtin.c
===
--- clang/test/CodeGen/dump-struct-builtin.c
+++ clang/test/CodeGen/dump-struct-builtin.c
@@ -4,95 +4,148 @@
 #include 
 
 // CHECK: @__const.unit1.a = private unnamed_addr constant %struct.U1A { i16 12 }, align 2
-// CHECK-NEXT: [[STRUCT_STR_U1:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U1A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U1:@[0-9]+]] = private unnamed_addr constant [19 x i8] c"short a = %hd\0A\00", align 1
-// CHECK-NEXT: [[END_STRUCT_U1:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
+// CHECK-NEXT: [[STRUCT_STR_U1:@[0-9]+]] = private unnamed_addr constant [12 x i8] c"struct U1A \00", align 1
+// CHECK-NEXT: [[STRUCT_L_BRACE_U1:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"{\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U1:@[0-9]+]] = private unnamed_addr constant [15 x i8] c"short a = \00", align 1
+// CHECK-NEXT: [[FORMAT_U1:@[0-9]+]] = private unnamed_addr constant [5 x i8] c"%hd\0A\00", align 1
+// CHECK-NEXT: [[STRUCT_R_BRACE_U1:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit2.a = private unnamed_addr constant %struct.U2A { i16 12 }, align 2
-// CHECK-NEXT: [[STRUCT_STR_U2:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U2A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U2:@[0-9]+]] = private unnamed_addr constant [28 x i8] c"unsigned short a = %hu\0A\00", align 1
-// CHECK-NEXT: [[END_STRUCT_U2:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
+// CHECK-NEXT: [[STRUCT_STR_U2:@[0-9]+]] = private unnamed_addr constant [12 x i8] c"struct U2A \00", align 1
+// CHECK-NEXT: [[STRUCT_L_BRACE_U2:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"{\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U2:@[0-9]+]] = private unnamed_addr constant [24 x i8] c"unsigned short a = \00", align 1
+// CHECK-NEXT: [[FORMAT_U2:@[0-9]+]] = private unnamed_addr constant [5 x i8] c"%hu\0A\00", align 1
+// CHECK-NEXT: [[STRUCT_R_BRACE_U2:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit3.a = private unnamed_addr constant %struct.U3A { i32 12 }, align 4
-// CHECK-NEXT: [[STRUCT_STR_U3:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U3A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U3:@[0-9]+]] = private unnamed_addr constant [16 x i8] c"int a = %d\0A\00", align 1
-// CHECK-NEXT: [[END_STRUCT_U3:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
+// CHECK-NEXT: [[STRUCT_STR_U3:@[0-9]+]] = private unnamed_addr constant [12 x i8] c"struct U3A \00", align 1
+// CHECK-NEXT: [[STRUCT_L_BRACE_U3:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"{\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U3:@[0-9]+]] = private unnamed_addr constant [13 x i8] c"int a = \00", align 1
+// CHECK-NEXT: [[FORMAT_U3:@[0-9]+]] = private unnamed_addr constant [4 x i8] c"%d\0A\00", align 1
+// CHECK-NEXT: [[STRUCT_R_BRACE_U3:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit4.a = private unnamed_addr constant %struct.U4A { i32 12 }, align 4
-// CHECK-NEXT: [[STRUCT_STR_U4:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U4A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U4:@[0-9]+]] = private unnamed_addr constant [25 x i8] c"unsigned int a = %u\0A\00", align 1
-// CHECK-NEXT: [[END_STRUCT_U4:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
+// CHECK-NEXT: [[STRUCT_STR_U4:@[0-9]+]] = private unnamed_addr constant [12 x i8] c"struct U4A \00", align 1
+// CHECK-NEXT: [[STRUCT_L_BRACE_U4:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"{\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U4:@[0-9]+]] = private unnamed_addr constant [22 x i8] c"unsigned int a = \00", align 1
+// CHECK-NEXT: [[FORMAT_U4:@[0-9]+]] = private unnamed_addr constant [4 x i8] c"%u\0A\00", align 1
+// CHECK-NEXT: [[STRUCT_R_BRACE_U4:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit5.a = private unnamed_addr constant %struct.U5A { i64 12 }, align 8
-// CHECK-NEXT: [[STRUCT_STR_U5:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U5A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U5:@[0-9]+]] = private unnamed_addr constant [18 x i8] c"long a = %ld\0A\00", align 1
-// CHECK-NEXT: [[END_STRUCT_U5:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
+// CHECK-NEXT: [[STRUCT_STR_U5:@[0-9]+]] = private unnamed_addr constant [12 x i8] c"struct U5A \00", align 1
+// 

[PATCH] D122920: [Clang][CodeGen]Fix __builtin_dump_struct missing record type field name

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

> While we'd usually be happy to take the fix-in-hand and apply it, part of the 
> discussion on the other thread is whether to remove `__builtin_dump_struct` 
> entirely. Because of that, I don't think we should make substantial changes 
> in this area until it's clear we're keeping the builtin.

Thanks for take a look, I agree to wait for the outcome of the discussion


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122920/new/

https://reviews.llvm.org/D122920

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122920: [Clang][CodeGen]Fix __builtin_dump_struct missing record type field name

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

In D122920#3472379 , @asoffer wrote:

> In D122920#3471865 , @yihanaa wrote:
>
>> In D122920#3471654 , @erichkeane 
>> wrote:
>>
>>> @yihanaa : I'd suggest seeing the conversation that @rsmith @aaron.ballman 
>>> and I are having about this builtin here: https://reviews.llvm.org/D124221
>>>
>>> In general it looks like the three of us think that this builtin needs an 
>>> overhaul in implementation/functionality in order to be further useful. 
>>> While we very much appreciate your attempts to try to improve it, we 
>>> believe there needs to be larger-scale changes.
>>
>> Thanks for your reply, I would like to get involved and help if needed
>
> While I eagerly await `__builtin_reflect_struct`, this change still provides 
> significant value in fixing a regression with `__builtin_dump_struct`. Should 
> we proceed with this change and that proposal independently?

Agree


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122920/new/

https://reviews.llvm.org/D122920

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122920: [Clang][CodeGen]Fix __builtin_dump_struct missing record type field name

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

In D122920#3472379 , @asoffer wrote:

> In D122920#3471865 , @yihanaa wrote:
>
>> In D122920#3471654 , @erichkeane 
>> wrote:
>>
>>> @yihanaa : I'd suggest seeing the conversation that @rsmith @aaron.ballman 
>>> and I are having about this builtin here: https://reviews.llvm.org/D124221
>>>
>>> In general it looks like the three of us think that this builtin needs an 
>>> overhaul in implementation/functionality in order to be further useful. 
>>> While we very much appreciate your attempts to try to improve it, we 
>>> believe there needs to be larger-scale changes.
>>
>> Thanks for your reply, I would like to get involved and help if needed
>
> While I eagerly await `__builtin_reflect_struct`, this change still provides 
> significant value in fixing a regression with `__builtin_dump_struct`. Should 
> we proceed with this change and that proposal independently?

Agree


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122920/new/

https://reviews.llvm.org/D122920

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124221: Add new builtin __builtin_reflect_struct.

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

Thank you for inviting me to a discussion on this topic. I have something very 
confusing as follows:

- If the developer already knows all fields info in some struct, wyh doesn't he 
write a simple function to do the printing(like: print_struct_foo(const struct 
foo *); )? It might be easier to write a printing  function.
- If the struct has many levels of nesting, and the developers just want to 
output the details of the struct for debugging purposes, then I think they 
might just need something like the 'p' command in LLDB.
- If we want to support these printing functions, and make this builtin 
flexible and general, I think reflection is a good idea. But we may want to 
make this builtin support C/C++ or other languages supported by Clang at the 
same time. I have a not-so-sophisticated (perhaps silly-sounding) idea: this 
builtin might be one or more C-style functions in order to be usable in many 
different languages. This family of functions supports getting the number of 
struct fields, as well as their details (and possibly getting the value of the 
field by getting the subscript). like:

  int __builtin_reflect_struct_member_count(const void *struct_addr, unsigned 
*count);
  int __builtin_reflect_struct_member_detail(const void *struct_addr, unsigned 
index, char *name, unsigned *bitfield, int *is_unnamed);
  int __builtin_reflect_struct_member_ptr(const void *struct_addr, unsigned 
index, void **ptr);

Maybe it also can support nested struct?

em, my idea looks really stupid, don't laugh at me, hahahaha~~~


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124221/new/

https://reviews.llvm.org/D124221

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122920: [Clang][CodeGen]Fix __builtin_dump_struct missing record type field name

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

In D122920#3471654 , @erichkeane 
wrote:

> @yihanaa : I'd suggest seeing the conversation that @rsmith @aaron.ballman 
> and I are having about this builtin here: https://reviews.llvm.org/D124221
>
> In general it looks like the three of us think that this builtin needs an 
> overhaul in implementation/functionality in order to be further useful. While 
> we very much appreciate your attempts to try to improve it, we believe there 
> needs to be larger-scale changes.

Thanks for your reply, I would like to get involved and help if needed


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122920/new/

https://reviews.llvm.org/D122920

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122920: [Clang][CodeGen]Fix __builtin_dump_struct missing record type field name

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

Should we use PrintingPolicy.Indentation instead of 4 hardcoded in dumpRecord?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122920/new/

https://reviews.llvm.org/D122920

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124221: Add new builtin __builtin_reflect_struct.

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:420
+auto *FD = IFD ? IFD->getAnonField() : dyn_cast(D);
+if (!FD || (FD->isUnnamedBitfield() || FD->isAnonymousStructOrUnion()))
+  continue;

erichkeane wrote:
> rsmith wrote:
> > erichkeane wrote:
> > > rsmith wrote:
> > > > erichkeane wrote:
> > > > > rsmith wrote:
> > > > > > erichkeane wrote:
> > > > > > > Losing the unnamed bitfield is unfortunate, and I the 'dump 
> > > > > > > struct' handles.  As is losing the anonymous struct/union.
> > > > > > > 
> > > > > > > Also, how does this handle 'record type' members?  Does the user 
> > > > > > > have to know the inner type already?  If they already know that 
> > > > > > > much information about the type, they wouldn't really need this, 
> > > > > > > right?
> > > > > > If `__builtin_dump_struct` includes unnamed bitfields, that's a bug 
> > > > > > in that builtin.
> > > > > > 
> > > > > > In general, the user function gets given the bases and members with 
> > > > > > the right types, so they can use an overload set or a template to 
> > > > > > dispatch based on that type. See the SemaCXX test for a basic 
> > > > > > example of that.
> > > > > I thought it did?  For the use case I see `__builtin_dump_struct` 
> > > > > having, I would think printing the existence of unnamed bitfields to 
> > > > > be quite useful.
> > > > > 
> > > > > 
> > > > Why? They're not part of the value, they're just padding.
> > > They are lexically part of the type and are part of what makes up the 
> > > 'size' of the thing. I would expect something dumping the type to be as 
> > > consistent lexically as possible.
> > Looks like `__builtin_dump_struct` currently includes them *and* prints a 
> > value (whatever garbage happens to be in those padding bits). That's 
> > obviously a bug.
> O.o! Yeah, printing the value is nonsense.
I think the user should be informed somehow that there is an unnamed bitfield 
here


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124221/new/

https://reviews.llvm.org/D124221

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122920: [Clang][CodeGen]Fix __builtin_dump_struct missing record type field name

[Wang Yihan via Phabricator via cfe-commits]

yihanaa updated this revision to Diff 424718.
yihanaa added a comment.

Optimize the code according to @ rsmith's suggestion, and use 
analyze_printf::PrintfSpecifier::fixType instead of this static DenseMap to 
find the printf format specifier.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122920/new/

https://reviews.llvm.org/D122920

Files:
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/test/CodeGen/dump-struct-builtin.c

Index: clang/test/CodeGen/dump-struct-builtin.c
===
--- clang/test/CodeGen/dump-struct-builtin.c
+++ clang/test/CodeGen/dump-struct-builtin.c
@@ -4,95 +4,148 @@
 #include 
 
 // CHECK: @__const.unit1.a = private unnamed_addr constant %struct.U1A { i16 12 }, align 2
-// CHECK-NEXT: [[STRUCT_STR_U1:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U1A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U1:@[0-9]+]] = private unnamed_addr constant [19 x i8] c"short a = %hd\0A\00", align 1
-// CHECK-NEXT: [[END_STRUCT_U1:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
+// CHECK-NEXT: [[STRUCT_STR_U1:@[0-9]+]] = private unnamed_addr constant [12 x i8] c"struct U1A \00", align 1
+// CHECK-NEXT: [[STRUCT_L_BRACE_U1:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"{\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U1:@[0-9]+]] = private unnamed_addr constant [15 x i8] c"short a = \00", align 1
+// CHECK-NEXT: [[FORMAT_U1:@[0-9]+]] = private unnamed_addr constant [5 x i8] c"%hd\0A\00", align 1
+// CHECK-NEXT: [[STRUCT_R_BRACE_U1:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit2.a = private unnamed_addr constant %struct.U2A { i16 12 }, align 2
-// CHECK-NEXT: [[STRUCT_STR_U2:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U2A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U2:@[0-9]+]] = private unnamed_addr constant [28 x i8] c"unsigned short a = %hu\0A\00", align 1
-// CHECK-NEXT: [[END_STRUCT_U2:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
+// CHECK-NEXT: [[STRUCT_STR_U2:@[0-9]+]] = private unnamed_addr constant [12 x i8] c"struct U2A \00", align 1
+// CHECK-NEXT: [[STRUCT_L_BRACE_U2:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"{\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U2:@[0-9]+]] = private unnamed_addr constant [24 x i8] c"unsigned short a = \00", align 1
+// CHECK-NEXT: [[FORMAT_U2:@[0-9]+]] = private unnamed_addr constant [5 x i8] c"%hu\0A\00", align 1
+// CHECK-NEXT: [[STRUCT_R_BRACE_U2:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit3.a = private unnamed_addr constant %struct.U3A { i32 12 }, align 4
-// CHECK-NEXT: [[STRUCT_STR_U3:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U3A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U3:@[0-9]+]] = private unnamed_addr constant [16 x i8] c"int a = %d\0A\00", align 1
-// CHECK-NEXT: [[END_STRUCT_U3:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
+// CHECK-NEXT: [[STRUCT_STR_U3:@[0-9]+]] = private unnamed_addr constant [12 x i8] c"struct U3A \00", align 1
+// CHECK-NEXT: [[STRUCT_L_BRACE_U3:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"{\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U3:@[0-9]+]] = private unnamed_addr constant [13 x i8] c"int a = \00", align 1
+// CHECK-NEXT: [[FORMAT_U3:@[0-9]+]] = private unnamed_addr constant [4 x i8] c"%d\0A\00", align 1
+// CHECK-NEXT: [[STRUCT_R_BRACE_U3:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit4.a = private unnamed_addr constant %struct.U4A { i32 12 }, align 4
-// CHECK-NEXT: [[STRUCT_STR_U4:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U4A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U4:@[0-9]+]] = private unnamed_addr constant [25 x i8] c"unsigned int a = %u\0A\00", align 1
-// CHECK-NEXT: [[END_STRUCT_U4:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
+// CHECK-NEXT: [[STRUCT_STR_U4:@[0-9]+]] = private unnamed_addr constant [12 x i8] c"struct U4A \00", align 1
+// CHECK-NEXT: [[STRUCT_L_BRACE_U4:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"{\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U4:@[0-9]+]] = private unnamed_addr constant [22 x i8] c"unsigned int a = \00", align 1
+// CHECK-NEXT: [[FORMAT_U4:@[0-9]+]] = private unnamed_addr constant [4 x i8] c"%u\0A\00", align 1
+// CHECK-NEXT: [[STRUCT_R_BRACE_U4:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit5.a = private unnamed_addr constant %struct.U5A { i64 12 }, align 8
-// CHECK-NEXT: [[STRUCT_STR_U5:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U5A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U5:@[0-9]+]] = private unnamed_addr constant [18 x i8] c"long a = %ld\0A\00", align 1
-// CHECK-NEXT: [[END_STRUCT_U5:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", 

[PATCH] D122920: [Clang][CodeGen]Fix __builtin_dump_struct missing record type field name

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2046-2048
 static llvm::Value *dumpRecord(CodeGenFunction , QualType RType,
LValue RecordLV, CharUnits Align,
+   bool dumpTypeName, llvm::FunctionCallee Func,

rsmith wrote:
> Instead of passing a flag that suppresses the first section of this function, 
> it would be clearer to split this up into two parts:
> 
> 1) A function that dumps a type name (`struct A` or `int` or whatever).
> 2) A function that dumps a struct value (`{x = 1, y = 2}`).
> 
> Then when printing the overall value, you can call (1) then (2), and when 
> printing each field, you can call (1), then print the field name, then call 
> (2) for a struct value (and do whatever else makes sense for other kinds of 
> values).
> Instead of passing a flag that suppresses the first section of this function, 
> it would be clearer to split this up into two parts:
> 
> 1) A function that dumps a type name (`struct A` or `int` or whatever).
> 2) A function that dumps a struct value (`{x = 1, y = 2}`).
> 
> Then when printing the overall value, you can call (1) then (2), and when 
> printing each field, you can call (1), then print the field name, then call 
> (2) for a struct value (and do whatever else makes sense for other kinds of 
> values).

Thanks for your review! I agree with your suggestion, and i try to improve code.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2071
   if (Types.empty()) {
 Types[Context.CharTy] = "%c";
 Types[Context.BoolTy] = "%d";

I have an idea, can we use  analyze_printf::PrintfSpecifier::fixType instead of 
this static DenseMap to find the printf format specifier? what do you all think?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122920/new/

https://reviews.llvm.org/D122920

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122920: [Clang][CodeGen]Fix __builtin_dump_struct missing record type field name

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

Could you please help me review the code？@rsmith，thanks


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122920/new/

https://reviews.llvm.org/D122920

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122248: [clang][CodeGen]Fix clang crash and add bitfield support in __builtin_dump_struct

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2113-2117
 if (CanonicalType->isRecordType()) {
-  TmpRes = dumpRecord(CGF, CanonicalType, FieldPtr, Align, Func, Lvl + 1);
+  TmpRes = dumpRecord(CGF, CanonicalType, FieldLV, Align, Func, Lvl + 1);
   Res = CGF.Builder.CreateAdd(TmpRes, Res);
   continue;
 }

yihanaa wrote:
> yihanaa wrote:
> > yihanaa wrote:
> > > rsmith wrote:
> > > > After this patch, this case no longer prints the field name. Eg: 
> > > > https://godbolt.org/z/o7vcbWaEf
> > > > 
> > > > ```
> > > > #include 
> > > > 
> > > > struct A {};
> > > > 
> > > > struct B {
> > > > A a;
> > > > };
> > > > 
> > > > 
> > > > int main() {
> > > > B x;
> > > > __builtin_dump_struct(, );
> > > > }
> > > > ```
> > > > 
> > > > now prints:
> > > > 
> > > > ```
> > > > B {
> > > > A {
> > > > }
> > > > }
> > > > ```
> > > > 
> > > > This seems like an important regression; please can you take a look? 
> > > > This also suggests to me that there's a hole in our test coverage.
> > > Thanks for taking the time to review my patch and writing the Compiler 
> > > Explorer examples, I'll take a look at this  problem
> > > After this patch, this case no longer prints the field name. Eg: 
> > > https://godbolt.org/z/o7vcbWaEf
> > > 
> > > ```
> > > #include 
> > > 
> > > struct A {};
> > > 
> > > struct B {
> > > A a;
> > > };
> > > 
> > > 
> > > int main() {
> > > B x;
> > > __builtin_dump_struct(, );
> > > }
> > > ```
> > > 
> > > now prints:
> > > 
> > > ```
> > > B {
> > > A {
> > > }
> > > }
> > > ```
> > > 
> > > This seems like an important regression; please can you take a look? This 
> > > also suggests to me that there's a hole in our test coverage.
> > 
> > I'm sorry for introducing this bug.  Do you think the following is correct 
> > behavior? If yes I will try to fix it.
> > ```
> > struct B {
> > struct A a = {
> > }
> > }
> > ```
> I have submitted a patch to fix this. https://reviews.llvm.org/D122920
> I have submitted a patch to fix this. https://reviews.llvm.org/D122920

Please can you take a review？@rsmith


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122248/new/

https://reviews.llvm.org/D122248

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122920: [Clang][CodeGen]Fix __builtin_dump_struct missing record type field name

[Wang Yihan via Phabricator via cfe-commits]

yihanaa updated this revision to Diff 420254.
yihanaa added a comment.

Improve comment


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122920/new/

https://reviews.llvm.org/D122920

Files:
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/test/CodeGen/dump-struct-builtin.c

Index: clang/test/CodeGen/dump-struct-builtin.c
===
--- clang/test/CodeGen/dump-struct-builtin.c
+++ clang/test/CodeGen/dump-struct-builtin.c
@@ -5,95 +5,122 @@
 
 // CHECK: @__const.unit1.a = private unnamed_addr constant %struct.U1A { i16 12 }, align 2
 // CHECK-NEXT: [[STRUCT_STR_U1:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U1A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U1:@[0-9]+]] = private unnamed_addr constant [19 x i8] c"short a = %hd\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U1:@[0-9]+]] = private unnamed_addr constant [15 x i8] c"short a = \00", align 1
+// CHECK-NEXT: [[FORMAT_U1:@[0-9]+]] = private unnamed_addr constant [5 x i8] c"%hd\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U1:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit2.a = private unnamed_addr constant %struct.U2A { i16 12 }, align 2
 // CHECK-NEXT: [[STRUCT_STR_U2:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U2A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U2:@[0-9]+]] = private unnamed_addr constant [28 x i8] c"unsigned short a = %hu\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U2:@[0-9]+]] = private unnamed_addr constant [24 x i8] c"unsigned short a = \00", align 1
+// CHECK-NEXT: [[FORMAT_U2:@[0-9]+]] = private unnamed_addr constant [5 x i8] c"%hu\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U2:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit3.a = private unnamed_addr constant %struct.U3A { i32 12 }, align 4
 // CHECK-NEXT: [[STRUCT_STR_U3:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U3A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U3:@[0-9]+]] = private unnamed_addr constant [16 x i8] c"int a = %d\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U3:@[0-9]+]] = private unnamed_addr constant [13 x i8] c"int a = \00", align 1
+// CHECK-NEXT: [[FORMAT_U3:@[0-9]+]] = private unnamed_addr constant [4 x i8] c"%d\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U3:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit4.a = private unnamed_addr constant %struct.U4A { i32 12 }, align 4
 // CHECK-NEXT: [[STRUCT_STR_U4:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U4A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U4:@[0-9]+]] = private unnamed_addr constant [25 x i8] c"unsigned int a = %u\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U4:@[0-9]+]] = private unnamed_addr constant [22 x i8] c"unsigned int a = \00", align 1
+// CHECK-NEXT: [[FORMAT_U4:@[0-9]+]] = private unnamed_addr constant [4 x i8] c"%u\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U4:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit5.a = private unnamed_addr constant %struct.U5A { i64 12 }, align 8
 // CHECK-NEXT: [[STRUCT_STR_U5:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U5A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U5:@[0-9]+]] = private unnamed_addr constant [18 x i8] c"long a = %ld\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U5:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"long a = \00", align 1
+// CHECK-NEXT: [[FORMAT_U5:@[0-9]+]] = private unnamed_addr constant [5 x i8] c"%ld\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U5:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit6.a = private unnamed_addr constant %struct.U6A { i64 12 }, align 8
 // CHECK-NEXT: [[STRUCT_STR_U6:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U6A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U6:@[0-9]+]] = private unnamed_addr constant [27 x i8] c"unsigned long a = %lu\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U6:@[0-9]+]] = private unnamed_addr constant [23 x i8] c"unsigned long a = \00", align 1
+// CHECK-NEXT: [[FORMAT_U6:@[0-9]+]] = private unnamed_addr constant [5 x i8] c"%lu\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U6:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit7.a = private unnamed_addr constant %struct.U7A { i64 12 }, align 8
 // CHECK-NEXT: [[STRUCT_STR_U7:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U7A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U7:@[0-9]+]] = private unnamed_addr constant [24 x i8] c"long long a = %lld\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U7:@[0-9]+]] = private unnamed_addr constant [19 x i8] c"long long a = \00", align 1
+// CHECK-NEXT: [[FORMAT_U7:@[0-9]+]] = private unnamed_addr constant [6 x i8] c"%lld\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U7:@[0-9]+]] = private 

[PATCH] D122920: [Clang][CodeGen]Fix __builtin_dump_struct missing record type field name

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2048
LValue RecordLV, CharUnits Align,
-   llvm::FunctionCallee Func, int Lvl) {
+   bool dumpTypeName, llvm::FunctionCallee Func,
+   int Lvl) {

yihanaa wrote:
> erichkeane wrote:
> > yihanaa wrote:
> > > erichkeane wrote:
> > > > Not a fan of bool parameters, they make calls harder to read.
> > > I can add comments for this parameter or this function, what do you 
> > > think, or do you have a better idea? I would like to improve it.
> > I prefer using an 'enum' for these sorts of things, we do that elsewhere.  
> > It seems that what you ACTUALLY care about here is whether this is a 'top 
> > level', and thus has a FieldName to print, right? So you actually want to 
> > dumpFieldName?  Or am I still misunderstanding something?
> > I prefer using an 'enum' for these sorts of things, we do that elsewhere.  
> > It seems that what you ACTUALLY care about here is whether this is a 'top 
> > level', and thus has a FieldName to print, right? So you actually want to 
> > dumpFieldName?  Or am I still misunderstanding something?
> 
> Yeah, you are right. A little  different is that this patch will DO NOT dump 
> the typename, field name and '=' in recursive dumpRecord call. 
Yeah, you are right. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122920/new/

https://reviews.llvm.org/D122920

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122920: [Clang][CodeGen]Fix __builtin_dump_struct missing record type field name

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

I'll just add that in the recursive call, for record type, it will just print 
what's between {...}
Eg:

  int printf(const char *, ...);
  
  struct A {
  int x;
  };
  
  struct B {
  struct A a;
  struct C {
int b;
union X {
  int i;
  int j;
} z;
  } bar;
  };
  
  
  int main() {
  struct B x = {0};
  __builtin_dump_struct(, );
  }

output:

  struct B {
  struct A a = {
  int x = 0
  }
  struct C bar = {
  int b = 0
  union X z = {
  int i = 0
  int j = 0
  }
  }
  }


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122920/new/

https://reviews.llvm.org/D122920

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122920: [Clang][CodeGen]Fix __builtin_dump_struct missing record type field name

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2048
LValue RecordLV, CharUnits Align,
-   llvm::FunctionCallee Func, int Lvl) {
+   bool dumpTypeName, llvm::FunctionCallee Func,
+   int Lvl) {

erichkeane wrote:
> yihanaa wrote:
> > erichkeane wrote:
> > > Not a fan of bool parameters, they make calls harder to read.
> > I can add comments for this parameter or this function, what do you think, 
> > or do you have a better idea? I would like to improve it.
> I prefer using an 'enum' for these sorts of things, we do that elsewhere.  It 
> seems that what you ACTUALLY care about here is whether this is a 'top 
> level', and thus has a FieldName to print, right? So you actually want to 
> dumpFieldName?  Or am I still misunderstanding something?
> I prefer using an 'enum' for these sorts of things, we do that elsewhere.  It 
> seems that what you ACTUALLY care about here is whether this is a 'top 
> level', and thus has a FieldName to print, right? So you actually want to 
> dumpFieldName?  Or am I still misunderstanding something?

Yeah, you are right. A little  different is that this patch will DO NOT dump 
the typename, field name and '=' in recursive dumpRecord call. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122920/new/

https://reviews.llvm.org/D122920

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122920: [Clang][CodeGen]Fix __builtin_dump_struct missing record type field name

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2048
LValue RecordLV, CharUnits Align,
-   llvm::FunctionCallee Func, int Lvl) {
+   bool dumpTypeName, llvm::FunctionCallee Func,
+   int Lvl) {

erichkeane wrote:
> Not a fan of bool parameters, they make calls harder to read.
I can add comments for this parameter or this function, what do you think, or 
do you have a better idea? I would like to improve it.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2141
+  TmpRes =
+  dumpRecord(CGF, CanonicalType, FieldLV, Align, false, Func, Lvl + 1);
   Res = CGF.Builder.CreateAdd(TmpRes, Res);

erichkeane wrote:
> 
Agree!I will modify it later



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2705
 LValue RecordLV = MakeAddrLValue(RecordPtr, Arg0Type, Arg0Align);
-Value *Res = dumpRecord(*this, Arg0Type, RecordLV, Arg0Align,
+Value *Res = dumpRecord(*this, Arg0Type, RecordLV, Arg0Align, true,
 {LLVMFuncType, Func}, 0);

erichkeane wrote:
> 
Agree！


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122920/new/

https://reviews.llvm.org/D122920

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122920: [Clang][CodeGen]Fix __builtin_dump_struct missing record type field name

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

In D122920#3426556 , @erichkeane 
wrote:

> I don't particularly get what the 'fix' is here to Richard's issue.  
> Additionally, I think this is showing me the absolute desperate need we have 
> for some sort of 'text-checking' tests for this feature, checking vs the IR 
> is too unreadable.

Thanks for review!  The issue that @rsmith reported was when the struct field 
is a record type, the __builtin_dumpr_struct no longer prints the field name. 
Eg:
The code:

  #include 
  
  struct A {
  int x;
  };
  
  struct B {
  struct A a;
  };
  
  
  int main() {
  struct B x = {0};
  __builtin_dump_struct(, );
  }

The clang trunk:(This missing field name 'a' for type struct A), our expected 
output is 'struct A a = {')

  struct B {
  struct A {
  int x = 0
  }
  }

After this patch:

  struct B {
  struct A a = {
  int x = 0
  }
  }

I try to improve this comment. And do you think it would be better to name this 
boolean 'includeTypeName'?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122920/new/

https://reviews.llvm.org/D122920

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122920: [Clang][CodeGen]Fix __builtin_dump_struct missing record type field name

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

Can you guys please help me review this patch?  


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122920/new/

https://reviews.llvm.org/D122920

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122248: [clang][CodeGen]Fix clang crash and add bitfield support in __builtin_dump_struct

[Wang Yihan via Phabricator via cfe-commits]

yihanaa marked an inline comment as done.
yihanaa added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2113-2117
 if (CanonicalType->isRecordType()) {
-  TmpRes = dumpRecord(CGF, CanonicalType, FieldPtr, Align, Func, Lvl + 1);
+  TmpRes = dumpRecord(CGF, CanonicalType, FieldLV, Align, Func, Lvl + 1);
   Res = CGF.Builder.CreateAdd(TmpRes, Res);
   continue;
 }

yihanaa wrote:
> yihanaa wrote:
> > rsmith wrote:
> > > After this patch, this case no longer prints the field name. Eg: 
> > > https://godbolt.org/z/o7vcbWaEf
> > > 
> > > ```
> > > #include 
> > > 
> > > struct A {};
> > > 
> > > struct B {
> > > A a;
> > > };
> > > 
> > > 
> > > int main() {
> > > B x;
> > > __builtin_dump_struct(, );
> > > }
> > > ```
> > > 
> > > now prints:
> > > 
> > > ```
> > > B {
> > > A {
> > > }
> > > }
> > > ```
> > > 
> > > This seems like an important regression; please can you take a look? This 
> > > also suggests to me that there's a hole in our test coverage.
> > Thanks for taking the time to review my patch and writing the Compiler 
> > Explorer examples, I'll take a look at this  problem
> > After this patch, this case no longer prints the field name. Eg: 
> > https://godbolt.org/z/o7vcbWaEf
> > 
> > ```
> > #include 
> > 
> > struct A {};
> > 
> > struct B {
> > A a;
> > };
> > 
> > 
> > int main() {
> > B x;
> > __builtin_dump_struct(, );
> > }
> > ```
> > 
> > now prints:
> > 
> > ```
> > B {
> > A {
> > }
> > }
> > ```
> > 
> > This seems like an important regression; please can you take a look? This 
> > also suggests to me that there's a hole in our test coverage.
> 
> I'm sorry for introducing this bug.  Do you think the following is correct 
> behavior? If yes I will try to fix it.
> ```
> struct B {
> struct A a = {
> }
> }
> ```
I have submitted a patch to fix this. https://reviews.llvm.org/D122920


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122248/new/

https://reviews.llvm.org/D122248

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122920: [Clang][CodeGen]Fix __builtin_dump_struct missing record type field name

[Wang Yihan via Phabricator via cfe-commits]

yihanaa updated this revision to Diff 419928.
yihanaa added a comment.

Reformat code


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122920/new/

https://reviews.llvm.org/D122920

Files:
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/test/CodeGen/dump-struct-builtin.c

Index: clang/test/CodeGen/dump-struct-builtin.c
===
--- clang/test/CodeGen/dump-struct-builtin.c
+++ clang/test/CodeGen/dump-struct-builtin.c
@@ -5,95 +5,122 @@
 
 // CHECK: @__const.unit1.a = private unnamed_addr constant %struct.U1A { i16 12 }, align 2
 // CHECK-NEXT: [[STRUCT_STR_U1:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U1A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U1:@[0-9]+]] = private unnamed_addr constant [19 x i8] c"short a = %hd\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U1:@[0-9]+]] = private unnamed_addr constant [15 x i8] c"short a = \00", align 1
+// CHECK-NEXT: [[FORMAT_U1:@[0-9]+]] = private unnamed_addr constant [5 x i8] c"%hd\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U1:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit2.a = private unnamed_addr constant %struct.U2A { i16 12 }, align 2
 // CHECK-NEXT: [[STRUCT_STR_U2:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U2A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U2:@[0-9]+]] = private unnamed_addr constant [28 x i8] c"unsigned short a = %hu\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U2:@[0-9]+]] = private unnamed_addr constant [24 x i8] c"unsigned short a = \00", align 1
+// CHECK-NEXT: [[FORMAT_U2:@[0-9]+]] = private unnamed_addr constant [5 x i8] c"%hu\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U2:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit3.a = private unnamed_addr constant %struct.U3A { i32 12 }, align 4
 // CHECK-NEXT: [[STRUCT_STR_U3:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U3A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U3:@[0-9]+]] = private unnamed_addr constant [16 x i8] c"int a = %d\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U3:@[0-9]+]] = private unnamed_addr constant [13 x i8] c"int a = \00", align 1
+// CHECK-NEXT: [[FORMAT_U3:@[0-9]+]] = private unnamed_addr constant [4 x i8] c"%d\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U3:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit4.a = private unnamed_addr constant %struct.U4A { i32 12 }, align 4
 // CHECK-NEXT: [[STRUCT_STR_U4:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U4A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U4:@[0-9]+]] = private unnamed_addr constant [25 x i8] c"unsigned int a = %u\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U4:@[0-9]+]] = private unnamed_addr constant [22 x i8] c"unsigned int a = \00", align 1
+// CHECK-NEXT: [[FORMAT_U4:@[0-9]+]] = private unnamed_addr constant [4 x i8] c"%u\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U4:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit5.a = private unnamed_addr constant %struct.U5A { i64 12 }, align 8
 // CHECK-NEXT: [[STRUCT_STR_U5:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U5A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U5:@[0-9]+]] = private unnamed_addr constant [18 x i8] c"long a = %ld\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U5:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"long a = \00", align 1
+// CHECK-NEXT: [[FORMAT_U5:@[0-9]+]] = private unnamed_addr constant [5 x i8] c"%ld\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U5:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit6.a = private unnamed_addr constant %struct.U6A { i64 12 }, align 8
 // CHECK-NEXT: [[STRUCT_STR_U6:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U6A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U6:@[0-9]+]] = private unnamed_addr constant [27 x i8] c"unsigned long a = %lu\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U6:@[0-9]+]] = private unnamed_addr constant [23 x i8] c"unsigned long a = \00", align 1
+// CHECK-NEXT: [[FORMAT_U6:@[0-9]+]] = private unnamed_addr constant [5 x i8] c"%lu\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U6:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit7.a = private unnamed_addr constant %struct.U7A { i64 12 }, align 8
 // CHECK-NEXT: [[STRUCT_STR_U7:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U7A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U7:@[0-9]+]] = private unnamed_addr constant [24 x i8] c"long long a = %lld\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U7:@[0-9]+]] = private unnamed_addr constant [19 x i8] c"long long a = \00", align 1
+// CHECK-NEXT: [[FORMAT_U7:@[0-9]+]] = private unnamed_addr constant [6 x i8] c"%lld\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U7:@[0-9]+]] = private 

[PATCH] D122920: [Clang][CodeGen]Fix __builtin_dump_struct missing record type field name

[Wang Yihan via Phabricator via cfe-commits]

yihanaa updated this revision to Diff 419923.
yihanaa added a comment.

Reformat code.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122920/new/

https://reviews.llvm.org/D122920

Files:
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/test/CodeGen/dump-struct-builtin.c

Index: clang/test/CodeGen/dump-struct-builtin.c
===
--- clang/test/CodeGen/dump-struct-builtin.c
+++ clang/test/CodeGen/dump-struct-builtin.c
@@ -5,95 +5,122 @@
 
 // CHECK: @__const.unit1.a = private unnamed_addr constant %struct.U1A { i16 12 }, align 2
 // CHECK-NEXT: [[STRUCT_STR_U1:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U1A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U1:@[0-9]+]] = private unnamed_addr constant [19 x i8] c"short a = %hd\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U1:@[0-9]+]] = private unnamed_addr constant [15 x i8] c"short a = \00", align 1
+// CHECK-NEXT: [[FORMAT_U1:@[0-9]+]] = private unnamed_addr constant [5 x i8] c"%hd\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U1:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit2.a = private unnamed_addr constant %struct.U2A { i16 12 }, align 2
 // CHECK-NEXT: [[STRUCT_STR_U2:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U2A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U2:@[0-9]+]] = private unnamed_addr constant [28 x i8] c"unsigned short a = %hu\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U2:@[0-9]+]] = private unnamed_addr constant [24 x i8] c"unsigned short a = \00", align 1
+// CHECK-NEXT: [[FORMAT_U2:@[0-9]+]] = private unnamed_addr constant [5 x i8] c"%hu\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U2:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit3.a = private unnamed_addr constant %struct.U3A { i32 12 }, align 4
 // CHECK-NEXT: [[STRUCT_STR_U3:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U3A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U3:@[0-9]+]] = private unnamed_addr constant [16 x i8] c"int a = %d\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U3:@[0-9]+]] = private unnamed_addr constant [13 x i8] c"int a = \00", align 1
+// CHECK-NEXT: [[FORMAT_U3:@[0-9]+]] = private unnamed_addr constant [4 x i8] c"%d\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U3:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit4.a = private unnamed_addr constant %struct.U4A { i32 12 }, align 4
 // CHECK-NEXT: [[STRUCT_STR_U4:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U4A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U4:@[0-9]+]] = private unnamed_addr constant [25 x i8] c"unsigned int a = %u\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U4:@[0-9]+]] = private unnamed_addr constant [22 x i8] c"unsigned int a = \00", align 1
+// CHECK-NEXT: [[FORMAT_U4:@[0-9]+]] = private unnamed_addr constant [4 x i8] c"%u\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U4:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit5.a = private unnamed_addr constant %struct.U5A { i64 12 }, align 8
 // CHECK-NEXT: [[STRUCT_STR_U5:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U5A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U5:@[0-9]+]] = private unnamed_addr constant [18 x i8] c"long a = %ld\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U5:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"long a = \00", align 1
+// CHECK-NEXT: [[FORMAT_U5:@[0-9]+]] = private unnamed_addr constant [5 x i8] c"%ld\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U5:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit6.a = private unnamed_addr constant %struct.U6A { i64 12 }, align 8
 // CHECK-NEXT: [[STRUCT_STR_U6:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U6A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U6:@[0-9]+]] = private unnamed_addr constant [27 x i8] c"unsigned long a = %lu\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U6:@[0-9]+]] = private unnamed_addr constant [23 x i8] c"unsigned long a = \00", align 1
+// CHECK-NEXT: [[FORMAT_U6:@[0-9]+]] = private unnamed_addr constant [5 x i8] c"%lu\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U6:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit7.a = private unnamed_addr constant %struct.U7A { i64 12 }, align 8
 // CHECK-NEXT: [[STRUCT_STR_U7:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U7A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U7:@[0-9]+]] = private unnamed_addr constant [24 x i8] c"long long a = %lld\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U7:@[0-9]+]] = private unnamed_addr constant [19 x i8] c"long long a = \00", align 1
+// CHECK-NEXT: [[FORMAT_U7:@[0-9]+]] = private unnamed_addr constant [6 x i8] c"%lld\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U7:@[0-9]+]] = private 

[PATCH] D122920: [Clang][CodeGen]Fix __builtin_dump_struct missing record type field name

[Wang Yihan via Phabricator via cfe-commits]

yihanaa created this revision.
yihanaa added reviewers: rsmith, erichkeane, aaron.ballman.
yihanaa added a project: clang.
Herald added a project: All.
yihanaa requested review of this revision.
Herald added a subscriber: cfe-commits.

Thanks for @rsmith to point this. I'm sorry for introducing this bug.
See @rsmith 's comment in https://reviews.llvm.org/D122248
Eg:(By @rsmith ) https://godbolt.org/z/o7vcbWaEf
I have added a test case
struct:

  struct U19A {
  int a;
};
struct U19B {
  struct U19A a;
};
  
struct U19B a = {
  .a.a = 2022
};

Dump result:

  struct U19B {
  struct U19A a = {
  int a = 2022
  }
  }


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D122920

Files:
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/test/CodeGen/dump-struct-builtin.c

Index: clang/test/CodeGen/dump-struct-builtin.c
===
--- clang/test/CodeGen/dump-struct-builtin.c
+++ clang/test/CodeGen/dump-struct-builtin.c
@@ -5,95 +5,122 @@
 
 // CHECK: @__const.unit1.a = private unnamed_addr constant %struct.U1A { i16 12 }, align 2
 // CHECK-NEXT: [[STRUCT_STR_U1:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U1A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U1:@[0-9]+]] = private unnamed_addr constant [19 x i8] c"short a = %hd\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U1:@[0-9]+]] = private unnamed_addr constant [15 x i8] c"short a = \00", align 1
+// CHECK-NEXT: [[FORMAT_U1:@[0-9]+]] = private unnamed_addr constant [5 x i8] c"%hd\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U1:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit2.a = private unnamed_addr constant %struct.U2A { i16 12 }, align 2
 // CHECK-NEXT: [[STRUCT_STR_U2:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U2A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U2:@[0-9]+]] = private unnamed_addr constant [28 x i8] c"unsigned short a = %hu\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U2:@[0-9]+]] = private unnamed_addr constant [24 x i8] c"unsigned short a = \00", align 1
+// CHECK-NEXT: [[FORMAT_U2:@[0-9]+]] = private unnamed_addr constant [5 x i8] c"%hu\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U2:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit3.a = private unnamed_addr constant %struct.U3A { i32 12 }, align 4
 // CHECK-NEXT: [[STRUCT_STR_U3:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U3A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U3:@[0-9]+]] = private unnamed_addr constant [16 x i8] c"int a = %d\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U3:@[0-9]+]] = private unnamed_addr constant [13 x i8] c"int a = \00", align 1
+// CHECK-NEXT: [[FORMAT_U3:@[0-9]+]] = private unnamed_addr constant [4 x i8] c"%d\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U3:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit4.a = private unnamed_addr constant %struct.U4A { i32 12 }, align 4
 // CHECK-NEXT: [[STRUCT_STR_U4:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U4A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U4:@[0-9]+]] = private unnamed_addr constant [25 x i8] c"unsigned int a = %u\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U4:@[0-9]+]] = private unnamed_addr constant [22 x i8] c"unsigned int a = \00", align 1
+// CHECK-NEXT: [[FORMAT_U4:@[0-9]+]] = private unnamed_addr constant [4 x i8] c"%u\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U4:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit5.a = private unnamed_addr constant %struct.U5A { i64 12 }, align 8
 // CHECK-NEXT: [[STRUCT_STR_U5:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U5A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U5:@[0-9]+]] = private unnamed_addr constant [18 x i8] c"long a = %ld\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U5:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"long a = \00", align 1
+// CHECK-NEXT: [[FORMAT_U5:@[0-9]+]] = private unnamed_addr constant [5 x i8] c"%ld\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U5:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit6.a = private unnamed_addr constant %struct.U6A { i64 12 }, align 8
 // CHECK-NEXT: [[STRUCT_STR_U6:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U6A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U6:@[0-9]+]] = private unnamed_addr constant [27 x i8] c"unsigned long a = %lu\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U6:@[0-9]+]] = private unnamed_addr constant [23 x i8] c"unsigned long a = \00", align 1
+// CHECK-NEXT: [[FORMAT_U6:@[0-9]+]] = private unnamed_addr constant [5 x i8] c"%lu\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U6:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit7.a = private unnamed_addr constant %struct.U7A { i64 12 

[PATCH] D122248: [clang][CodeGen]Fix clang crash and add bitfield support in __builtin_dump_struct

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2113-2117
 if (CanonicalType->isRecordType()) {
-  TmpRes = dumpRecord(CGF, CanonicalType, FieldPtr, Align, Func, Lvl + 1);
+  TmpRes = dumpRecord(CGF, CanonicalType, FieldLV, Align, Func, Lvl + 1);
   Res = CGF.Builder.CreateAdd(TmpRes, Res);
   continue;
 }

yihanaa wrote:
> rsmith wrote:
> > After this patch, this case no longer prints the field name. Eg: 
> > https://godbolt.org/z/o7vcbWaEf
> > 
> > ```
> > #include 
> > 
> > struct A {};
> > 
> > struct B {
> > A a;
> > };
> > 
> > 
> > int main() {
> > B x;
> > __builtin_dump_struct(, );
> > }
> > ```
> > 
> > now prints:
> > 
> > ```
> > B {
> > A {
> > }
> > }
> > ```
> > 
> > This seems like an important regression; please can you take a look? This 
> > also suggests to me that there's a hole in our test coverage.
> Thanks for taking the time to review my patch and writing the Compiler 
> Explorer examples, I'll take a look at this  problem
> After this patch, this case no longer prints the field name. Eg: 
> https://godbolt.org/z/o7vcbWaEf
> 
> ```
> #include 
> 
> struct A {};
> 
> struct B {
> A a;
> };
> 
> 
> int main() {
> B x;
> __builtin_dump_struct(, );
> }
> ```
> 
> now prints:
> 
> ```
> B {
> A {
> }
> }
> ```
> 
> This seems like an important regression; please can you take a look? This 
> also suggests to me that there's a hole in our test coverage.

I'm sorry for introducing this bug.  Do you think the following is correct 
behavior? If yes I will try to fix it.
```
struct B {
struct A a = {
}
}
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122248/new/

https://reviews.llvm.org/D122248

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122248: [clang][CodeGen]Fix clang crash and add bitfield support in __builtin_dump_struct

[Wang Yihan via Phabricator via cfe-commits]

yihanaa marked 3 inline comments as done.
yihanaa added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2113-2117
 if (CanonicalType->isRecordType()) {
-  TmpRes = dumpRecord(CGF, CanonicalType, FieldPtr, Align, Func, Lvl + 1);
+  TmpRes = dumpRecord(CGF, CanonicalType, FieldLV, Align, Func, Lvl + 1);
   Res = CGF.Builder.CreateAdd(TmpRes, Res);
   continue;
 }

rsmith wrote:
> After this patch, this case no longer prints the field name. Eg: 
> https://godbolt.org/z/o7vcbWaEf
> 
> ```
> #include 
> 
> struct A {};
> 
> struct B {
> A a;
> };
> 
> 
> int main() {
> B x;
> __builtin_dump_struct(, );
> }
> ```
> 
> now prints:
> 
> ```
> B {
> A {
> }
> }
> ```
> 
> This seems like an important regression; please can you take a look? This 
> also suggests to me that there's a hole in our test coverage.
Thanks for taking the time to review my patch and writing the Compiler Explorer 
examples, I'll take a look at this  problem


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122248/new/

https://reviews.llvm.org/D122248

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122822: [Clang][CodeGen]Add constant array support for __builtin_dump_sturct

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

In D122822#3422463 , @erichkeane 
wrote:

> In D122822#3421089 , @yihanaa wrote:
>
>> I also agree that this is a useful debugging builtin, and no program logic 
>> should be forced to depend on the output of this builtin. It's just used to 
>> print out some useful debugging information
>
> FWIW, @rsmith is the clang-codeowner, so he's the one to convince (and 
> @rjmccall is also a very senior reviewer).  So I wouldn't be able to approve 
> this patch without their consent/agreement.

Thank you for your information, @erichkeane I really hope we can find a 
solution and reach a consensus


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122822/new/

https://reviews.llvm.org/D122822

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122822: [Clang][CodeGen]Add constant array support for __builtin_dump_sturct

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2052
 Types[Context.BoolTy] = "%d";
 Types[Context.SignedCharTy] = "%hhd";
 Types[Context.UnsignedCharTy] = "%hhu";

I think this types should have been promoted according to the integer 
promotions.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122822/new/

https://reviews.llvm.org/D122822

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122822: [Clang][CodeGen]Add constant array support for __builtin_dump_sturct

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

I also agree that this is a useful debugging builtin, and no program logic 
should be forced to depend on the output of this builtin. It's just used to 
print out some useful debugging information


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122822/new/

https://reviews.llvm.org/D122822

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122822: [Clang][CodeGen]Add constant array support for __builtin_dump_sturct

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2048
+  QualType Type) {
+  static llvm::DenseMap Types;
   if (Types.empty()) {

erichkeane wrote:
> yihanaa wrote:
> > erichkeane wrote:
> > > yihanaa wrote:
> > > > erichkeane wrote:
> > > > > yihanaa wrote:
> > > > > > yihanaa wrote:
> > > > > > > erichkeane wrote:
> > > > > > > > yihanaa wrote:
> > > > > > > > > erichkeane wrote:
> > > > > > > > > > yihanaa wrote:
> > > > > > > > > > > erichkeane wrote:
> > > > > > > > > > > > Instead of this initialization, can we make this a 
> > > > > > > > > > > > constexpr constructor/collection of some sort and 
> > > > > > > > > > > > instantiate it inline?  
> > > > > > > > > > > > Instead of this initialization, can we make this a 
> > > > > > > > > > > > constexpr constructor/collection of some sort and 
> > > > > > > > > > > > instantiate it inline?  
> > > > > > > > > > > 
> > > > > > > > > > > I don't have a good idea because it relies on Context to 
> > > > > > > > > > > get some types like const char *
> > > > > > > > > > We could at minimum do a in inline-initializer instead of 
> > > > > > > > > > the branch below.
> > > > > > > > > I still haven't come up with a good idea to do it
> > > > > > > > Hrmph, `DenseMap` doesn't seem to have an init list ctor?!
> > > > > > > > 
> > > > > > > > Since this is a finite, known at compile time list of low 'N', 
> > > > > > > > I'd suggest just making it `static std::array > > > > > > > StringRef>> Types = {{Context.CharTy, "%c"},...`.
> > > > > > > > 
> > > > > > > > Then just doing a llvm::find_if.  The list is small enough I 
> > > > > > > > suspect the DenseMap overhead is more than the use of 
> > > > > > > > `llvm::find_if`.
> > > > > > > Haha,DenseMap also has no constexpr ctor. I think the time 
> > > > > > > complexity of these two is the same. 
> > > > > > The Context.CharTy... etc. are not static, So we might need a 
> > > > > > static value instead of it, and compare it with QualType
> > > > > They don't have to be static?  This should do 'magic static' 
> > > > > initialization.
> > > > I can't agree more. it might be a type identifier, such as 
> > > > getAsString()'s return value
> > > Actually, I don't think the collection can be static!  When we re-use the 
> > > same process for multiple TUs, we get a new ASTContext.  SO this would 
> > > result in us having types that don't necessarily 'match'.  I think this 
> > > collection/checks need to either be stored in ASTContext, or in an 
> > > 'if-else' tree here.
> > I also don't think it should be maintained in this place. Shouldn't this be 
> > better maintained together with the printf parameter checking part, what do 
> > you think?
> I'm unfamiliar with what you mean.
> 
> HOWEVER, the comments by @rsmith and @rjmccall I think are blocking on this 
> feature/functionality anyway.  So we might still wish to refactor this 
> builtin significantly based on what they come up with.
Ah, I think the way I described it is not correct, my thought is, can this 
getDumpStructFormatSpecifier similar function be maintained in somewhere like 
FormatString.h


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122822/new/

https://reviews.llvm.org/D122822

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122822: [Clang][CodeGen]Add constant array support for __builtin_dump_sturct

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2048
+  QualType Type) {
+  static llvm::DenseMap Types;
   if (Types.empty()) {

erichkeane wrote:
> yihanaa wrote:
> > erichkeane wrote:
> > > yihanaa wrote:
> > > > yihanaa wrote:
> > > > > erichkeane wrote:
> > > > > > yihanaa wrote:
> > > > > > > erichkeane wrote:
> > > > > > > > yihanaa wrote:
> > > > > > > > > erichkeane wrote:
> > > > > > > > > > Instead of this initialization, can we make this a 
> > > > > > > > > > constexpr constructor/collection of some sort and 
> > > > > > > > > > instantiate it inline?  
> > > > > > > > > > Instead of this initialization, can we make this a 
> > > > > > > > > > constexpr constructor/collection of some sort and 
> > > > > > > > > > instantiate it inline?  
> > > > > > > > > 
> > > > > > > > > I don't have a good idea because it relies on Context to get 
> > > > > > > > > some types like const char *
> > > > > > > > We could at minimum do a in inline-initializer instead of the 
> > > > > > > > branch below.
> > > > > > > I still haven't come up with a good idea to do it
> > > > > > Hrmph, `DenseMap` doesn't seem to have an init list ctor?!
> > > > > > 
> > > > > > Since this is a finite, known at compile time list of low 'N', I'd 
> > > > > > suggest just making it `static std::array > > > > > StringRef>> Types = {{Context.CharTy, "%c"},...`.
> > > > > > 
> > > > > > Then just doing a llvm::find_if.  The list is small enough I 
> > > > > > suspect the DenseMap overhead is more than the use of 
> > > > > > `llvm::find_if`.
> > > > > Haha,DenseMap also has no constexpr ctor. I think the time complexity 
> > > > > of these two is the same. 
> > > > The Context.CharTy... etc. are not static, So we might need a static 
> > > > value instead of it, and compare it with QualType
> > > They don't have to be static?  This should do 'magic static' 
> > > initialization.
> > I can't agree more. it might be a type identifier, such as getAsString()'s 
> > return value
> Actually, I don't think the collection can be static!  When we re-use the 
> same process for multiple TUs, we get a new ASTContext.  SO this would result 
> in us having types that don't necessarily 'match'.  I think this 
> collection/checks need to either be stored in ASTContext, or in an 'if-else' 
> tree here.
I also don't think it should be maintained in this place. Shouldn't this be 
better maintained together with the printf parameter checking part, what do you 
think?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122822/new/

https://reviews.llvm.org/D122822

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122822: [Clang][CodeGen]Add constant array support for __builtin_dump_sturct

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

In D122822#3420162 , @rsmith wrote:

> I'm concerned about the direction of this patch given @rjmccall's comments on 
> https://reviews.llvm.org/D112626 -- presumably the way we'd want to address 
> those comments would be to convert a `__builtin_dump_struct(a, f)` call into 
> a single `f("...",  a.x, a.y, a.z)` call in Sema,  and that approach doesn't 
> seem compatible with generating code to loop over array elements.
>
> I'm also concerned that this builtin is making a lot of design decisions on 
> behalf of the programmer, meaning that either it does exactly what you want 
> or it's not suitable for your use and there's not much you can do about it. A 
> different design that let the programmer choose whether to recurse into 
> structs and arrays and similarly let the programmer choose how to format the 
> fields would seem preferable, but I'm not confident there's a good way to 
> expose that in C (though in C++ there seem to be good designs to choose 
> from). As an example of how that manifests here, the choice to print each 
> element as a separate line for a struct member of type `const char str[256];` 
> is probably going to make the output very unreadable, but choosing to print 
> every array of `char` using `"%s"` will be equally bad for arrays that don't 
> contain text -- the "right" answer for a dumping facility probably involves 
> checking whether the array contains only printable characters and trimming a 
> trailing sequence of zeroes, but that seems like vastly more complexity than 
> we'd want in code generated by a builtin.

I'm also a newbie and only gradually integrated into the community with the 
help of @erichkeane , @aaron.ballman and others many times, tthis problem may 
be a bit difficult for me at the moment, but I'd be like to do a bigger 
overhaul of the whole builtin if that would make it more great.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122822/new/

https://reviews.llvm.org/D122822

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122822: [Clang][CodeGen]Add constant array support for __builtin_dump_sturct

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2048
+  QualType Type) {
+  static llvm::DenseMap Types;
   if (Types.empty()) {

erichkeane wrote:
> yihanaa wrote:
> > yihanaa wrote:
> > > erichkeane wrote:
> > > > yihanaa wrote:
> > > > > erichkeane wrote:
> > > > > > yihanaa wrote:
> > > > > > > erichkeane wrote:
> > > > > > > > Instead of this initialization, can we make this a constexpr 
> > > > > > > > constructor/collection of some sort and instantiate it inline?  
> > > > > > > > Instead of this initialization, can we make this a constexpr 
> > > > > > > > constructor/collection of some sort and instantiate it inline?  
> > > > > > > 
> > > > > > > I don't have a good idea because it relies on Context to get some 
> > > > > > > types like const char *
> > > > > > We could at minimum do a in inline-initializer instead of the 
> > > > > > branch below.
> > > > > I still haven't come up with a good idea to do it
> > > > Hrmph, `DenseMap` doesn't seem to have an init list ctor?!
> > > > 
> > > > Since this is a finite, known at compile time list of low 'N', I'd 
> > > > suggest just making it `static std::array> 
> > > > Types = {{Context.CharTy, "%c"},...`.
> > > > 
> > > > Then just doing a llvm::find_if.  The list is small enough I suspect 
> > > > the DenseMap overhead is more than the use of `llvm::find_if`.
> > > Haha,DenseMap also has no constexpr ctor. I think the time complexity of 
> > > these two is the same. 
> > The Context.CharTy... etc. are not static, So we might need a static value 
> > instead of it, and compare it with QualType
> They don't have to be static?  This should do 'magic static' initialization.
I can't agree more. it might be a type identifier, such as getAsString()'s 
return value


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122822/new/

https://reviews.llvm.org/D122822

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122822: [Clang][CodeGen]Add constant array support for __builtin_dump_sturct

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2048
+  QualType Type) {
+  static llvm::DenseMap Types;
   if (Types.empty()) {

yihanaa wrote:
> erichkeane wrote:
> > yihanaa wrote:
> > > erichkeane wrote:
> > > > yihanaa wrote:
> > > > > erichkeane wrote:
> > > > > > Instead of this initialization, can we make this a constexpr 
> > > > > > constructor/collection of some sort and instantiate it inline?  
> > > > > > Instead of this initialization, can we make this a constexpr 
> > > > > > constructor/collection of some sort and instantiate it inline?  
> > > > > 
> > > > > I don't have a good idea because it relies on Context to get some 
> > > > > types like const char *
> > > > We could at minimum do a in inline-initializer instead of the branch 
> > > > below.
> > > I still haven't come up with a good idea to do it
> > Hrmph, `DenseMap` doesn't seem to have an init list ctor?!
> > 
> > Since this is a finite, known at compile time list of low 'N', I'd suggest 
> > just making it `static std::array> Types = 
> > {{Context.CharTy, "%c"},...`.
> > 
> > Then just doing a llvm::find_if.  The list is small enough I suspect the 
> > DenseMap overhead is more than the use of `llvm::find_if`.
> Haha,DenseMap also has no constexpr ctor. I think the time complexity of 
> these two is the same. 
The Context.CharTy... etc. are not static, So we might need a static value 
instead of it, and compare it with QualType


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122822/new/

https://reviews.llvm.org/D122822

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122822: [Clang][CodeGen]Add constant array support for __builtin_dump_sturct

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2048
+  QualType Type) {
+  static llvm::DenseMap Types;
   if (Types.empty()) {

erichkeane wrote:
> yihanaa wrote:
> > erichkeane wrote:
> > > yihanaa wrote:
> > > > erichkeane wrote:
> > > > > Instead of this initialization, can we make this a constexpr 
> > > > > constructor/collection of some sort and instantiate it inline?  
> > > > > Instead of this initialization, can we make this a constexpr 
> > > > > constructor/collection of some sort and instantiate it inline?  
> > > > 
> > > > I don't have a good idea because it relies on Context to get some types 
> > > > like const char *
> > > We could at minimum do a in inline-initializer instead of the branch 
> > > below.
> > I still haven't come up with a good idea to do it
> Hrmph, `DenseMap` doesn't seem to have an init list ctor?!
> 
> Since this is a finite, known at compile time list of low 'N', I'd suggest 
> just making it `static std::array> Types = 
> {{Context.CharTy, "%c"},...`.
> 
> Then just doing a llvm::find_if.  The list is small enough I suspect the 
> DenseMap overhead is more than the use of `llvm::find_if`.
Haha,DenseMap also has no constexpr ctor. I think the time complexity of these 
two is the same. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122822/new/

https://reviews.llvm.org/D122822

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122822: [Clang][CodeGen]Add constant array support for __builtin_dump_sturct

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2048
+  QualType Type) {
+  static llvm::DenseMap Types;
   if (Types.empty()) {

erichkeane wrote:
> yihanaa wrote:
> > erichkeane wrote:
> > > Instead of this initialization, can we make this a constexpr 
> > > constructor/collection of some sort and instantiate it inline?  
> > > Instead of this initialization, can we make this a constexpr 
> > > constructor/collection of some sort and instantiate it inline?  
> > 
> > I don't have a good idea because it relies on Context to get some types 
> > like const char *
> We could at minimum do a in inline-initializer instead of the branch below.
I still haven't come up with a good idea to do it


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122822/new/

https://reviews.llvm.org/D122822

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122822: [Clang][CodeGen]Add constant array support for __builtin_dump_sturct

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

There is another question, which way should I print for an empty array?

#1: Same as non empty array

  int[0] a = [
  ]

#2:

  int[0] a = []

The #1's format is more uniform, and the #2 maybe looks a little clearer. What 
do you think? @erichkeane


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122822/new/

https://reviews.llvm.org/D122822

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122822: [Clang][CodeGen]Add constant array support for __builtin_dump_sturct

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2071
+  if (Types.find(Type) == Types.end())
+return Types[Context.VoidPtrTy];
+  return Types[Type];

erichkeane wrote:
> yihanaa wrote:
> > erichkeane wrote:
> > > When can we hit this case?  Does this mean that any types we dont 
> > > understand we are just treating as void ptrs?  
> > The previous version treated it as a void *ptr for unsupported types, such 
> > as arrays (before this patch) and __int128. This is the case i saw in an 
> > issue. I also think that for unsupported types, we should generate a 
> > clearer message saying "This type is temporarily not supported", do you 
> > have any good ideas?
> A diagnostic about ''TYPE' not yet supported by __bultin...' probably isn't a 
> bad idea if we have the ability.
I think maybe we can emit an error(or warning? in this way, it may be necessary 
to handle those unsupported types) diag msg, but that might be worth a separate 
commit. What do you think?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122822/new/

https://reviews.llvm.org/D122822

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122822: [Clang][CodeGen]Add constant array support for __builtin_dump_sturct

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

Thanks for your suggestion @erichkeane !




Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2048
+  QualType Type) {
+  static llvm::DenseMap Types;
   if (Types.empty()) {

erichkeane wrote:
> Instead of this initialization, can we make this a constexpr 
> constructor/collection of some sort and instantiate it inline?  
> Instead of this initialization, can we make this a constexpr 
> constructor/collection of some sort and instantiate it inline?  

I don't have a good idea because it relies on Context to get some types like 
const char *



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2071
+  if (Types.find(Type) == Types.end())
+return Types[Context.VoidPtrTy];
+  return Types[Type];

erichkeane wrote:
> When can we hit this case?  Does this mean that any types we dont understand 
> we are just treating as void ptrs?  
The previous version treated it as a void *ptr for unsupported types, such as 
arrays (before this patch) and __int128. This is the case i saw in an issue. I 
also think that for unsupported types, we should generate a clearer message 
saying "This type is temporarily not supported", do you have any good ideas?



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2119
+  // But if the array length is constant, we can suppress that.
+  if (llvm::ConstantInt *CL = dyn_cast(Length)) {
+// ...and if it's constant zero, we can just skip the entire thing.

erichkeane wrote:
> This branch doesn't seem right?  We are in a `ConstantArrayType`, which means 
> we DEFINITELY know the length.  Additionally, `Length` is constructed 
> directly from a `llvm::ConstantInt::get`, so we already know this answer.
I agree with you



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2138
+
+  if (CheckZeroLength) {
+llvm::Value *isEmpty =

erichkeane wrote:
> We already know if this is a 'zero' length array, we should probably just not 
> emit IR at all in that case.
Good idea!



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2204
+  if (inArray) {
+GString = CGF.Builder.CreateGlobalStringPtr("{\n");
+  } else {

erichkeane wrote:
> instead of `inArray` you probably should have something a little more 
> descriptive here, it seems what you really need is something like, 
> `includeTypeName`, preferably as an enum to make it more clear at the caller 
> side.
I think that's a good idea for clarity.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2257
+
+if (CanonicalType->isConstantArrayType()) {
+  GString = CGF.Builder.CreateGlobalStringPtr(

erichkeane wrote:
> Do we want to do anything for the OTHER array types?  
I understand struct fields must have a constant size, and I see" 'variable 
length array in structure' extension will never be supported" from clang's 
diagnostics



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2827
 LValue RecordLV = MakeAddrLValue(RecordPtr, Arg0Type, Arg0Align);
-Value *Res = dumpRecord(*this, Arg0Type, RecordLV, Arg0Align,
+Value *Res = dumpRecord(*this, Arg0Type, RecordLV, Arg0Align, false,
 {LLVMFuncType, Func}, 0);

erichkeane wrote:
> Typically we do a comment to explain what bool parms mean.  Alternatively, if 
> instead you could create an enum of some sort that would better describe the 
> 'false', it wouldlikely be more helpful.  (same on 2252).
I agree!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122822/new/

https://reviews.llvm.org/D122822

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122822: [Clang][CodeGen]Add constant array support for __builtin_dump_sturct

[Wang Yihan via Phabricator via cfe-commits]

yihanaa created this revision.
yihanaa added reviewers: aaron.ballman, erichkeane, MaskRay.
yihanaa added a project: clang.
Herald added a subscriber: StephenFan.
Herald added a project: All.
yihanaa requested review of this revision.
Herald added a subscriber: cfe-commits.

Add constant array support for __builtin_dump_sturct. For N-dimensional arrays, 
the new dumpArray function dumps the elements of the array by generating N 
layers of 'for' loops.
for example:
The struct:

  struct Foo {
int x;
  };
  
  struct S {
int a[3];
int b[2][2];
struct Foo c[2];
  };

The dump result:

  struct S {
  int[3] a = [
  [0] = 1
  [1] = 2
  [2] = 3
  ]
  int[2][2] b = [
  [0] = [
  [0] = 1
  [1] = 2
  ]
  [1] = [
  [0] = 3
  [1] = 4
  ]
  ]
  struct Foo[2] c = [
  [0] = {
  int x = 1
  }
  [1] = {
  int x = 2
  }
  ]
  }


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D122822

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/test/CodeGen/dump-struct-builtin.c

Index: clang/test/CodeGen/dump-struct-builtin.c
===
--- clang/test/CodeGen/dump-struct-builtin.c
+++ clang/test/CodeGen/dump-struct-builtin.c
@@ -76,7 +76,11 @@
 
 // CHECK: @__const.unit15.a = private unnamed_addr constant %struct.U15A { [3 x i32] [i32 1, i32 2, i32 3] }, align 4
 // CHECK-NEXT: [[STRUCT_STR_U15:@[0-9]+]] = private unnamed_addr constant [15 x i8] c"struct U15A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U15:@[0-9]+]] = private unnamed_addr constant [19 x i8] c"int[3] a = %p\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U15:@[0-9]+]] = private unnamed_addr constant [16 x i8] c"int[3] a = \00", align 1
+// CHECK-NEXT: [[ARRAY_L_SQUARE_U15:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"[\0A\00", align 1
+// CHECK-NEXT: [[ARRAY_INDEX_U15:@[0-9]+]] = private unnamed_addr constant [17 x i8] c"[%ld] = \00", align 1
+// CHECK-NEXT: [[ARRAY_ELEMENT_U15:@[0-9]+]] = private unnamed_addr constant [4 x i8] c"%d\0A\00", align 1
+// CHECK-NEXT: [[ARRAY_R_SQUARE_U15:@[0-9]+]] = private unnamed_addr constant [7 x i8] c"]\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U15:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit16.a = private unnamed_addr constant %struct.U16A { i8 12 }, align 1
@@ -94,6 +98,18 @@
 // CHECK-NEXT: [[FIELD_U18:@[0-9]+]] = private unnamed_addr constant [25 x i8] c"long double a = %Lf\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U18:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
+// CHECK: @__const.unit19.a = private unnamed_addr constant %struct.T19A { {{.*}} }, align 4
+// CHECK-NEXT: [[STRUCT_STR_U19:@[0-9]+]] = private unnamed_addr constant [15 x i8] c"struct T19A {\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U19:@[0-9]+]] = private unnamed_addr constant [19 x i8] c"int[2][2] b = \00", align 1
+// CHECK-NEXT: [[ARRAY_L_SQUARE_U19:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"[\0A\00", align 1
+// CHECK-NEXT: [[ARRAY_INDEX_U19:@[0-9]+]] = private unnamed_addr constant [17 x i8] c"[%ld] = \00", align 1
+// CHECK-NEXT: [[ARRAY_L_SQUARE2_U19:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"[\0A\00", align 1
+// CHECK-NEXT: [[ARRAY_INDEX2_U19:@[0-9]+]] = private unnamed_addr constant [21 x i8] c"[%ld] = \00", align 1
+// CHECK-NEXT: [[ARRAY_ELEMENT_U19:@[0-9]+]] = private unnamed_addr constant [4 x i8] c"%d\0A\00", align 1
+// CHECK-NEXT: [[ARRAY_R_SQUARE2_U19:@[0-9]+]] = private unnamed_addr constant [11 x i8] c"]\0A\00", align 1
+// CHECK-NEXT: [[ARRAY_R_SQUARE_U19:@[0-9]+]] = private unnamed_addr constant [7 x i8] c"]\0A\00", align 1
+// CHECK-NEXT: [[END_STRUCT_U19:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
+
 int printf(const char *fmt, ...) {
 return 0;
 }
@@ -107,8 +123,8 @@
   .a = 12,
   };
   // CHECK: call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([14 x i8], [14 x i8]* [[STRUCT_STR_U1]], i32 0, i32 0))
-  // CHECK: [[RES1:%.*]] = getelementptr inbounds %struct.U1A, %struct.U1A* %a, i32 0, i32 0
-  // CHECK: [[LOAD1:%.*]] = load i16, i16* [[RES1]],
+  // CHECK: [[RES1:%[−a−zA−Z._0-9]+]] = getelementptr inbounds %struct.U1A, %struct.U1A* %a, i32 0, i32 0
+  // CHECK: [[LOAD1:%[0-9]+]] = load i16, i16* [[RES1]],
   // CHECK: call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([19 x i8], [19 x i8]* [[FIELD_U1]], i32 0, i32 0), i16 [[LOAD1]])
   // CHECK: call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([3 x i8], [3 x i8]* [[END_STRUCT_U1]], i32 0, i32 0))
   __builtin_dump_struct(, );
@@ -124,8 +140,8 @@
   };
 
   // CHECK: call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([14 x i8], [14 x i8]* [[STRUCT_STR_U2]], i32 0, i32 

[PATCH] D122704: [Clang][CodeGen]Beautify dump format, add indent for nested struct and struct members

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

Thank you for the reviews @erichkeane.   I've applied for commit access and 
commit in 907d3acefc3bdd6eb83f21589c6473ca7e88b3eb 
.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122704/new/

https://reviews.llvm.org/D122704

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122704: [Clang][CodeGen]Beautify dump format, add indent for nested struct and struct members

[Wang Yihan via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG907d3acefc3b: [Clang][CodeGen]Beautify dump format, add 
indent for nested struct and struct… (authored by yihanaa).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122704/new/

https://reviews.llvm.org/D122704

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/test/CodeGen/dump-struct-builtin.c

Index: clang/test/CodeGen/dump-struct-builtin.c
===
--- clang/test/CodeGen/dump-struct-builtin.c
+++ clang/test/CodeGen/dump-struct-builtin.c
@@ -5,93 +5,93 @@
 
 // CHECK: @__const.unit1.a = private unnamed_addr constant %struct.U1A { i16 12 }, align 2
 // CHECK-NEXT: [[STRUCT_STR_U1:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U1A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U1:@[0-9]+]] = private unnamed_addr constant [15 x i8] c"short a = %hd\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U1:@[0-9]+]] = private unnamed_addr constant [19 x i8] c"short a = %hd\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U1:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit2.a = private unnamed_addr constant %struct.U2A { i16 12 }, align 2
 // CHECK-NEXT: [[STRUCT_STR_U2:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U2A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U2:@[0-9]+]] = private unnamed_addr constant [24 x i8] c"unsigned short a = %hu\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U2:@[0-9]+]] = private unnamed_addr constant [28 x i8] c"unsigned short a = %hu\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U2:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit3.a = private unnamed_addr constant %struct.U3A { i32 12 }, align 4
 // CHECK-NEXT: [[STRUCT_STR_U3:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U3A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U3:@[0-9]+]] = private unnamed_addr constant [12 x i8] c"int a = %d\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U3:@[0-9]+]] = private unnamed_addr constant [16 x i8] c"int a = %d\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U3:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit4.a = private unnamed_addr constant %struct.U4A { i32 12 }, align 4
 // CHECK-NEXT: [[STRUCT_STR_U4:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U4A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U4:@[0-9]+]] = private unnamed_addr constant [21 x i8] c"unsigned int a = %u\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U4:@[0-9]+]] = private unnamed_addr constant [25 x i8] c"unsigned int a = %u\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U4:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit5.a = private unnamed_addr constant %struct.U5A { i64 12 }, align 8
 // CHECK-NEXT: [[STRUCT_STR_U5:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U5A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U5:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"long a = %ld\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U5:@[0-9]+]] = private unnamed_addr constant [18 x i8] c"long a = %ld\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U5:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit6.a = private unnamed_addr constant %struct.U6A { i64 12 }, align 8
 // CHECK-NEXT: [[STRUCT_STR_U6:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U6A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U6:@[0-9]+]] = private unnamed_addr constant [23 x i8] c"unsigned long a = %lu\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U6:@[0-9]+]] = private unnamed_addr constant [27 x i8] c"unsigned long a = %lu\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U6:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit7.a = private unnamed_addr constant %struct.U7A { i64 12 }, align 8
 // CHECK-NEXT: [[STRUCT_STR_U7:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U7A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U7:@[0-9]+]] = private unnamed_addr constant [20 x i8] c"long long a = %lld\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U7:@[0-9]+]] = private unnamed_addr constant [24 x i8] c"long long a = %lld\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U7:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit8.a = private unnamed_addr constant %struct.U8A { i64 12 }, align 8
 // CHECK-NEXT: [[STRUCT_STR_U8:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U8A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U8:@[0-9]+]] = private unnamed_addr constant [29 x i8] c"unsigned long long a = %llu\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U8:@[0-9]+]] = private unnamed_addr constant [33 x i8] c"unsigned long long 

[PATCH] D122704: [Clang][CodeGen]Beautify dump format, add indent for nested struct and struct members

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

In D122704#3416538 , @erichkeane 
wrote:

> This seems fine to me.  I won't likely have time to commit this for a while, 
> but you should be able to apply for commit-rights now and do it yourself.

Sorry for taking so long to reply to you, thank you very much for your review 
and suggestions, and i have tried to apply for commit access from Chris and am 
waiting for a reply.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122704/new/

https://reviews.llvm.org/D122704

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122704: [Clang][CodeGen]Beautify dump format, add indent for nested struct and struct members

[Wang Yihan via Phabricator via cfe-commits]

yihanaa created this revision.
yihanaa added reviewers: erichkeane, aaron.ballman.
yihanaa added a project: clang.
Herald added a project: All.
yihanaa requested review of this revision.
Herald added a subscriber: cfe-commits.

Beautify dump format, add indent for nested struct and struct members, also fix 
test cases in dump-struct-builtin.c
for example:
struct:

  struct A {
int a;
struct B {
  int b;
  struct C {
struct D {
  int d;
  union E {
int x;
int y;
  } e;
} d;
int c;
  } c;
} b;
  };

Before:

  struct A {
  int a = 0
  struct B {
  int b = 0
  struct C {
  struct D {
  int d = 0
  union E {
  int x = 0
  int y = 0
  }
  }
  int c = 0
  }
  }
  }

After:

  struct A {
  int a = 0
  struct B {
  int b = 0
  struct C {
  struct D {
  int d = 0
  union E {
  int x = 0
  int y = 0
  }
  }
  int c = 0
  }
  }
  }


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D122704

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/test/CodeGen/dump-struct-builtin.c

Index: clang/test/CodeGen/dump-struct-builtin.c
===
--- clang/test/CodeGen/dump-struct-builtin.c
+++ clang/test/CodeGen/dump-struct-builtin.c
@@ -5,93 +5,93 @@
 
 // CHECK: @__const.unit1.a = private unnamed_addr constant %struct.U1A { i16 12 }, align 2
 // CHECK-NEXT: [[STRUCT_STR_U1:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U1A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U1:@[0-9]+]] = private unnamed_addr constant [15 x i8] c"short a = %hd\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U1:@[0-9]+]] = private unnamed_addr constant [19 x i8] c"short a = %hd\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U1:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit2.a = private unnamed_addr constant %struct.U2A { i16 12 }, align 2
 // CHECK-NEXT: [[STRUCT_STR_U2:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U2A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U2:@[0-9]+]] = private unnamed_addr constant [24 x i8] c"unsigned short a = %hu\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U2:@[0-9]+]] = private unnamed_addr constant [28 x i8] c"unsigned short a = %hu\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U2:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit3.a = private unnamed_addr constant %struct.U3A { i32 12 }, align 4
 // CHECK-NEXT: [[STRUCT_STR_U3:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U3A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U3:@[0-9]+]] = private unnamed_addr constant [12 x i8] c"int a = %d\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U3:@[0-9]+]] = private unnamed_addr constant [16 x i8] c"int a = %d\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U3:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit4.a = private unnamed_addr constant %struct.U4A { i32 12 }, align 4
 // CHECK-NEXT: [[STRUCT_STR_U4:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U4A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U4:@[0-9]+]] = private unnamed_addr constant [21 x i8] c"unsigned int a = %u\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U4:@[0-9]+]] = private unnamed_addr constant [25 x i8] c"unsigned int a = %u\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U4:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit5.a = private unnamed_addr constant %struct.U5A { i64 12 }, align 8
 // CHECK-NEXT: [[STRUCT_STR_U5:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U5A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U5:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"long a = %ld\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U5:@[0-9]+]] = private unnamed_addr constant [18 x i8] c"long a = %ld\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U5:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit6.a = private unnamed_addr constant %struct.U6A { i64 12 }, align 8
 // CHECK-NEXT: [[STRUCT_STR_U6:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U6A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U6:@[0-9]+]] = private unnamed_addr constant [23 x i8] c"unsigned long a = %lu\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U6:@[0-9]+]] = private unnamed_addr constant [27 x i8] c"unsigned long a = %lu\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U6:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit7.a = private unnamed_addr constant %struct.U7A { i64 12 }, align 8
 // CHECK-NEXT: 

[PATCH] D122670: [Clang][CodeGen]Remove anonymous tag locations

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

In D122670#3414574 , @erichkeane 
wrote:

> Still LGTM.

Thank you!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122670/new/

https://reviews.llvm.org/D122670

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122670: [Clang][CodeGen]Remove anonymous tag locations

[Wang Yihan via Phabricator via cfe-commits]

yihanaa updated this revision to Diff 418929.
yihanaa added a comment.

Add this change into ReleaseNotes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122670/new/

https://reviews.llvm.org/D122670

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/CodeGen/CGBuiltin.cpp


Index: clang/lib/CodeGen/CGBuiltin.cpp
===
--- clang/lib/CodeGen/CGBuiltin.cpp
+++ clang/lib/CodeGen/CGBuiltin.cpp
@@ -2050,8 +2050,10 @@
   RecordDecl *RD = RType->castAs()->getDecl()->getDefinition();
   std::string Pad = std::string(Lvl * 4, ' ');
 
+  PrintingPolicy Policy(Context.getLangOpts());
+  Policy.AnonymousTagLocations = false;
   Value *GString =
-  CGF.Builder.CreateGlobalStringPtr(RType.getAsString() + " {\n");
+  CGF.Builder.CreateGlobalStringPtr(RType.getAsString(Policy) + " {\n");
   Value *Res = CGF.Builder.CreateCall(Func, {GString});
 
   static llvm::DenseMap Types;
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -116,6 +116,7 @@
 - Improve __builtin_dump_struct:
   - Support bitfields in struct and union.
   - Improve the dump format, dump both bitwidth(if its a bitfield) and field 
value.
+  - Remove anonymous tag locations.
 
 New Compiler Flags
 --


Index: clang/lib/CodeGen/CGBuiltin.cpp
===
--- clang/lib/CodeGen/CGBuiltin.cpp
+++ clang/lib/CodeGen/CGBuiltin.cpp
@@ -2050,8 +2050,10 @@
   RecordDecl *RD = RType->castAs()->getDecl()->getDefinition();
   std::string Pad = std::string(Lvl * 4, ' ');
 
+  PrintingPolicy Policy(Context.getLangOpts());
+  Policy.AnonymousTagLocations = false;
   Value *GString =
-  CGF.Builder.CreateGlobalStringPtr(RType.getAsString() + " {\n");
+  CGF.Builder.CreateGlobalStringPtr(RType.getAsString(Policy) + " {\n");
   Value *Res = CGF.Builder.CreateCall(Func, {GString});
 
   static llvm::DenseMap Types;
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -116,6 +116,7 @@
 - Improve __builtin_dump_struct:
   - Support bitfields in struct and union.
   - Improve the dump format, dump both bitwidth(if its a bitfield) and field value.
+  - Remove anonymous tag locations.
 
 New Compiler Flags
 --
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122670: [Clang][CodeGen]Remove anonymous tag locations

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

In D122670#3414549 , @erichkeane 
wrote:

> I think Aaron wants release notes for just about everything :)  Mind tossing 
> one on this patch?  I'll commit for you after that.

Thanks @erichkeane ,i try to add this change into ReleaseNotes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122670/new/

https://reviews.llvm.org/D122670

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122670: [Clang][CodeGen]Remove anonymous tag locations

[Wang Yihan via Phabricator via cfe-commits]

yihanaa created this revision.
yihanaa added reviewers: erichkeane, aaron.ballman.
Herald added a project: All.
yihanaa requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Remove anonymous tag locations, powered by 'PrintingPolicy', @aaron.ballman 
once suggested removing this extra information in 
https://reviews.llvm.org/D122248
struct:

  struct S {
int a;
struct /* Anonymous*/ {
  int x;
} b;
int c;
  };

Before:

  struct S {
  int a = 0
  struct S::(unnamed at ./builtin_dump_struct.c:20:3) {
  int x = 0
  }
  int c = 0
  }

After:

  struct S {
  int a = 0
  struct S::(unnamed) {
  int x = 0
  }
  int c = 0
  }


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D122670

Files:
  clang/lib/CodeGen/CGBuiltin.cpp


Index: clang/lib/CodeGen/CGBuiltin.cpp
===
--- clang/lib/CodeGen/CGBuiltin.cpp
+++ clang/lib/CodeGen/CGBuiltin.cpp
@@ -2050,8 +2050,10 @@
   RecordDecl *RD = RType->castAs()->getDecl()->getDefinition();
   std::string Pad = std::string(Lvl * 4, ' ');
 
+  PrintingPolicy Policy(Context.getLangOpts());
+  Policy.AnonymousTagLocations = false;
   Value *GString =
-  CGF.Builder.CreateGlobalStringPtr(RType.getAsString() + " {\n");
+  CGF.Builder.CreateGlobalStringPtr(RType.getAsString(Policy) + " {\n");
   Value *Res = CGF.Builder.CreateCall(Func, {GString});
 
   static llvm::DenseMap Types;


Index: clang/lib/CodeGen/CGBuiltin.cpp
===
--- clang/lib/CodeGen/CGBuiltin.cpp
+++ clang/lib/CodeGen/CGBuiltin.cpp
@@ -2050,8 +2050,10 @@
   RecordDecl *RD = RType->castAs()->getDecl()->getDefinition();
   std::string Pad = std::string(Lvl * 4, ' ');
 
+  PrintingPolicy Policy(Context.getLangOpts());
+  Policy.AnonymousTagLocations = false;
   Value *GString =
-  CGF.Builder.CreateGlobalStringPtr(RType.getAsString() + " {\n");
+  CGF.Builder.CreateGlobalStringPtr(RType.getAsString(Policy) + " {\n");
   Value *Res = CGF.Builder.CreateCall(Func, {GString});
 
   static llvm::DenseMap Types;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122668: [Clang][doc][NFC]Remove duplicate items in ReleaseNotes

[Wang Yihan via Phabricator via cfe-commits]

yihanaa marked 2 inline comments as done.
yihanaa added a comment.

In D122668#3414445 , @erichkeane 
wrote:

> 2 small bits, otherwise LGTM.

Thanks for review @erichkeane ,i have update the diff, but i don’t have commit 
access, can you land this patch for me? Please use “wangyihan 
1135831...@qq.com” to commit the change.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122668/new/

https://reviews.llvm.org/D122668

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122668: [Clang][doc][NFC]Remove duplicate items in ReleaseNotes

[Wang Yihan via Phabricator via cfe-commits]

yihanaa updated this revision to Diff 418920.
yihanaa added a comment.

Fixed "Description" issue and removed useless newlines


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122668/new/

https://reviews.llvm.org/D122668

Files:
  clang/docs/ReleaseNotes.rst


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -90,7 +90,9 @@
   `51414 `_,
   `51416 `_,
   and `51641 `_.
-
+- The builtin function __builtin_dump_struct would crash clang when the target 
+  struct contains a bitfield. It now correctly handles bitfields.
+  This fixes Issue `Issue 54462 
`_.
 
 Improvements to Clang's diagnostics
 ^^^
@@ -107,10 +109,9 @@
 
 Non-comprehensive list of changes in this release
 -
-- The builtin function __builtin_dump_struct would crash clang when the target 
-  struct have bitfield. Now it fixed, and __builtin_dump_struct support dump
-  the bitwidth of bitfields.
-  This fixes `Issue 54462 
`_.
+- Improve __builtin_dump_struct:
+  - Support bitfields in struct and union.
+  - Improve the dump format, dump both bitwidth(if its a bitfield) and field 
value.
 
 New Compiler Flags
 --
@@ -152,12 +153,6 @@
 - The ``__declspec(naked)`` attribute can no longer be written on a member
   function in Microsoft compatibility mode, matching the behavior of cl.exe.
 
-- Improve __builtin_dump_struct:
-
-  - Support bitfields in struct and union.
-  
-  - Improve the dump format, dump both bitwidth(if its a bitfield) and field 
value.
-
 Windows Support
 ---
 


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -90,7 +90,9 @@
   `51414 `_,
   `51416 `_,
   and `51641 `_.
-
+- The builtin function __builtin_dump_struct would crash clang when the target 
+  struct contains a bitfield. It now correctly handles bitfields.
+  This fixes Issue `Issue 54462 `_.
 
 Improvements to Clang's diagnostics
 ^^^
@@ -107,10 +109,9 @@
 
 Non-comprehensive list of changes in this release
 -
-- The builtin function __builtin_dump_struct would crash clang when the target 
-  struct have bitfield. Now it fixed, and __builtin_dump_struct support dump
-  the bitwidth of bitfields.
-  This fixes `Issue 54462 `_.
+- Improve __builtin_dump_struct:
+  - Support bitfields in struct and union.
+  - Improve the dump format, dump both bitwidth(if its a bitfield) and field value.
 
 New Compiler Flags
 --
@@ -152,12 +153,6 @@
 - The ``__declspec(naked)`` attribute can no longer be written on a member
   function in Microsoft compatibility mode, matching the behavior of cl.exe.
 
-- Improve __builtin_dump_struct:
-
-  - Support bitfields in struct and union.
-  
-  - Improve the dump format, dump both bitwidth(if its a bitfield) and field value.
-
 Windows Support
 ---
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122668: [Clang][doc][NFC]Remove duplicate items in ReleaseNotes

[Wang Yihan via Phabricator via cfe-commits]

yihanaa created this revision.
yihanaa added reviewers: erichkeane, aaron.ballman.
Herald added a project: All.
yihanaa requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Remove duplicate items in ReleaseNotes for __builtin_dump_struct, the code 
changes int patch https://reviews.llvm.org/D122248


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D122668

Files:
  clang/docs/ReleaseNotes.rst


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -90,7 +90,9 @@
   `51414 `_,
   `51416 `_,
   and `51641 `_.
-
+- The builtin function __builtin_dump_struct would crash clang when the target 
+  struct have bitfield. Now it fixed.
+  This fixes Issue `Issue 54462 
`_.
 
 Improvements to Clang's diagnostics
 ^^^
@@ -107,10 +109,11 @@
 
 Non-comprehensive list of changes in this release
 -
-- The builtin function __builtin_dump_struct would crash clang when the target 
-  struct have bitfield. Now it fixed, and __builtin_dump_struct support dump
-  the bitwidth of bitfields.
-  This fixes `Issue 54462 
`_.
+- Improve __builtin_dump_struct:
+
+  - Support bitfields in struct and union.
+  
+  - Improve the dump format, dump both bitwidth(if its a bitfield) and field 
value.
 
 New Compiler Flags
 --
@@ -152,12 +155,6 @@
 - The ``__declspec(naked)`` attribute can no longer be written on a member
   function in Microsoft compatibility mode, matching the behavior of cl.exe.
 
-- Improve __builtin_dump_struct:
-
-  - Support bitfields in struct and union.
-  
-  - Improve the dump format, dump both bitwidth(if its a bitfield) and field 
value.
-
 Windows Support
 ---
 


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -90,7 +90,9 @@
   `51414 `_,
   `51416 `_,
   and `51641 `_.
-
+- The builtin function __builtin_dump_struct would crash clang when the target 
+  struct have bitfield. Now it fixed.
+  This fixes Issue `Issue 54462 `_.
 
 Improvements to Clang's diagnostics
 ^^^
@@ -107,10 +109,11 @@
 
 Non-comprehensive list of changes in this release
 -
-- The builtin function __builtin_dump_struct would crash clang when the target 
-  struct have bitfield. Now it fixed, and __builtin_dump_struct support dump
-  the bitwidth of bitfields.
-  This fixes `Issue 54462 `_.
+- Improve __builtin_dump_struct:
+
+  - Support bitfields in struct and union.
+  
+  - Improve the dump format, dump both bitwidth(if its a bitfield) and field value.
 
 New Compiler Flags
 --
@@ -152,12 +155,6 @@
 - The ``__declspec(naked)`` attribute can no longer be written on a member
   function in Microsoft compatibility mode, matching the behavior of cl.exe.
 
-- Improve __builtin_dump_struct:
-
-  - Support bitfields in struct and union.
-  
-  - Improve the dump format, dump both bitwidth(if its a bitfield) and field value.
-
 Windows Support
 ---
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122662: [Clang][CodeGen] Add constant array support for __builtin_dump_sturct, and beautify dump format

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

In D122662#3414364 , @erichkeane 
wrote:

> In D122662#3414319 , @yihanaa wrote:
>
>> In D122662#3414284 , @erichkeane 
>> wrote:
>>
>>> I'd also suggest splitting into the '3' things that you're trying to 
>>> accomplish above.  The CGBuiltin.cpp code has way too much going on to 
>>> reasonably review.
>>
>>
>>
>> In D122662#3414284 , @erichkeane 
>> wrote:
>>
>>> I'd also suggest splitting into the '3' things that you're trying to 
>>> accomplish above.  The CGBuiltin.cpp code has way too much going on to 
>>> reasonably review.
>>
>> Maybe I should split the third thing into another separate patch, the first 
>> thing needs to modify the indent when adding constant array support, that is 
>> the third thing (modify the indent), what do you think?
>
> I would suggest piling them from #3, to #2 to #1.  That is: "Change the 
> anonymous printing" in the first, add the 'sub-element-tabbing' in 2nd, and 
> 'add constant array support' in 3rd.

Thanks for the suggestion @erichkeane  , I am trying to do this


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122662/new/

https://reviews.llvm.org/D122662

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122662: [Clang][CodeGen] Add constant array support for __builtin_dump_sturct, and beautify dump format

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

In D122662#3414284 , @erichkeane 
wrote:

> I'd also suggest splitting into the '3' things that you're trying to 
> accomplish above.  The CGBuiltin.cpp code has way too much going on to 
> reasonably review.



In D122662#3414284 , @erichkeane 
wrote:

> I'd also suggest splitting into the '3' things that you're trying to 
> accomplish above.  The CGBuiltin.cpp code has way too much going on to 
> reasonably review.

Maybe I should split the third thing into another separate patch, the first 
thing needs to modify the indent when adding constant array support, that is 
the third thing (modify the indent), what do you think?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122662/new/

https://reviews.llvm.org/D122662

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122662: [Clang][CodeGen] Add constant array support for __builtin_dump_sturct, and beautify dump format

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

In D122662#3414284 , @erichkeane 
wrote:

> I'd also suggest splitting into the '3' things that you're trying to 
> accomplish above.  The CGBuiltin.cpp code has way too much going on to 
> reasonably review.

These three things are mainly done in a new function dumpArray, and the 
dumpRecord is modified.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122662/new/

https://reviews.llvm.org/D122662

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122662: [Clang][CodeGen] Add constant array support for __builtin_dump_sturct, and beautify dump format

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

In D122662#3414199 , @erichkeane 
wrote:

> This looks to be quite the large patch, can you split this up into individual 
> patches?  It isn't clear to me what all is going on everywhere.

Thanks @erichkeane ,i have splited this patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122662/new/

https://reviews.llvm.org/D122662

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122662: [Clang][CodeGen] Add constant array support for __builtin_dump_sturct, and beautify dump format

[Wang Yihan via Phabricator via cfe-commits]

yihanaa updated this revision to Diff 418901.
yihanaa added a comment.

split another patch to fix ReleaseNotes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122662/new/

https://reviews.llvm.org/D122662

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/test/CodeGen/dump-struct-builtin.c

Index: clang/test/CodeGen/dump-struct-builtin.c
===
--- clang/test/CodeGen/dump-struct-builtin.c
+++ clang/test/CodeGen/dump-struct-builtin.c
@@ -5,95 +5,111 @@
 
 // CHECK: @__const.unit1.a = private unnamed_addr constant %struct.U1A { i16 12 }, align 2
 // CHECK-NEXT: [[STRUCT_STR_U1:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U1A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U1:@[0-9]+]] = private unnamed_addr constant [15 x i8] c"short a = %hd\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U1:@[0-9]+]] = private unnamed_addr constant [19 x i8] c"short a = %hd\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U1:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit2.a = private unnamed_addr constant %struct.U2A { i16 12 }, align 2
 // CHECK-NEXT: [[STRUCT_STR_U2:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U2A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U2:@[0-9]+]] = private unnamed_addr constant [24 x i8] c"unsigned short a = %hu\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U2:@[0-9]+]] = private unnamed_addr constant [28 x i8] c"unsigned short a = %hu\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U2:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit3.a = private unnamed_addr constant %struct.U3A { i32 12 }, align 4
 // CHECK-NEXT: [[STRUCT_STR_U3:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U3A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U3:@[0-9]+]] = private unnamed_addr constant [12 x i8] c"int a = %d\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U3:@[0-9]+]] = private unnamed_addr constant [16 x i8] c"int a = %d\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U3:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit4.a = private unnamed_addr constant %struct.U4A { i32 12 }, align 4
 // CHECK-NEXT: [[STRUCT_STR_U4:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U4A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U4:@[0-9]+]] = private unnamed_addr constant [21 x i8] c"unsigned int a = %u\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U4:@[0-9]+]] = private unnamed_addr constant [25 x i8] c"unsigned int a = %u\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U4:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit5.a = private unnamed_addr constant %struct.U5A { i64 12 }, align 8
 // CHECK-NEXT: [[STRUCT_STR_U5:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U5A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U5:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"long a = %ld\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U5:@[0-9]+]] = private unnamed_addr constant [18 x i8] c"long a = %ld\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U5:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit6.a = private unnamed_addr constant %struct.U6A { i64 12 }, align 8
 // CHECK-NEXT: [[STRUCT_STR_U6:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U6A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U6:@[0-9]+]] = private unnamed_addr constant [23 x i8] c"unsigned long a = %lu\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U6:@[0-9]+]] = private unnamed_addr constant [27 x i8] c"unsigned long a = %lu\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U6:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit7.a = private unnamed_addr constant %struct.U7A { i64 12 }, align 8
 // CHECK-NEXT: [[STRUCT_STR_U7:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U7A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U7:@[0-9]+]] = private unnamed_addr constant [20 x i8] c"long long a = %lld\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U7:@[0-9]+]] = private unnamed_addr constant [24 x i8] c"long long a = %lld\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U7:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit8.a = private unnamed_addr constant %struct.U8A { i64 12 }, align 8
 // CHECK-NEXT: [[STRUCT_STR_U8:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U8A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U8:@[0-9]+]] = private unnamed_addr constant [29 x i8] c"unsigned long long a = %llu\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U8:@[0-9]+]] = private unnamed_addr constant [33 x i8] c"unsigned long long a = %llu\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U8:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: 

[PATCH] D122662: [Clang][CodeGen] Add constant array support for __builtin_dump_sturct, and beautify dump format

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

Thanks for review @erichkeane ,maybe i should split this patch.




Comment at: clang/docs/ReleaseNotes.rst:93
   and `51641 `_.
+- The builtin function __builtin_dump_struct would crash clang when the target 
+  struct have bitfield. Now it fixed.

erichkeane wrote:
> What is this from/for?  Is this left over from a previous patch?
The fixed by me in https://reviews.llvm.org/D122248, this modification appeared 
in multiple places in ReleaseNote, so I removed the redundant.



Comment at: clang/docs/ReleaseNotes.rst:113
 -
-- The builtin function __builtin_dump_struct would crash clang when the target 
-  struct have bitfield. Now it fixed, and __builtin_dump_struct support dump
-  the bitwidth of bitfields.
-  This fixes `Issue 54462 
`_.
+- The builtin function __builtin_dump_struct support dump constant array and 
the 
+  bitwidth of bitfields in a struct.

erichkeane wrote:
> "now supports dumping constant arrays" perhaps?
Yes，i add constant array support int this patch.



Comment at: clang/docs/ReleaseNotes.rst:114
+- The builtin function __builtin_dump_struct support dump constant array and 
the 
+  bitwidth of bitfields in a struct.
+  

erichkeane wrote:
> This should probably be a different entry than the bitfield one.
should i separate the two？



Comment at: clang/docs/ReleaseNotes.rst:155
 
-- Improve __builtin_dump_struct:
-

erichkeane wrote:
> Probably want a separate patch to fix these.
Okay


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122662/new/

https://reviews.llvm.org/D122662

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122662: [Clang][CodeGen] Add constant array support for __builtin_dump_sturct, and beautify dump format

[Wang Yihan via Phabricator via cfe-commits]

yihanaa updated this revision to Diff 418895.
yihanaa added a comment.

Remove unnecessary code format


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122662/new/

https://reviews.llvm.org/D122662

Files:
  clang/docs/LanguageExtensions.rst
  clang/docs/ReleaseNotes.rst
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/test/CodeGen/dump-struct-builtin.c

Index: clang/test/CodeGen/dump-struct-builtin.c
===
--- clang/test/CodeGen/dump-struct-builtin.c
+++ clang/test/CodeGen/dump-struct-builtin.c
@@ -5,95 +5,111 @@
 
 // CHECK: @__const.unit1.a = private unnamed_addr constant %struct.U1A { i16 12 }, align 2
 // CHECK-NEXT: [[STRUCT_STR_U1:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U1A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U1:@[0-9]+]] = private unnamed_addr constant [15 x i8] c"short a = %hd\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U1:@[0-9]+]] = private unnamed_addr constant [19 x i8] c"short a = %hd\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U1:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit2.a = private unnamed_addr constant %struct.U2A { i16 12 }, align 2
 // CHECK-NEXT: [[STRUCT_STR_U2:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U2A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U2:@[0-9]+]] = private unnamed_addr constant [24 x i8] c"unsigned short a = %hu\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U2:@[0-9]+]] = private unnamed_addr constant [28 x i8] c"unsigned short a = %hu\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U2:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit3.a = private unnamed_addr constant %struct.U3A { i32 12 }, align 4
 // CHECK-NEXT: [[STRUCT_STR_U3:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U3A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U3:@[0-9]+]] = private unnamed_addr constant [12 x i8] c"int a = %d\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U3:@[0-9]+]] = private unnamed_addr constant [16 x i8] c"int a = %d\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U3:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit4.a = private unnamed_addr constant %struct.U4A { i32 12 }, align 4
 // CHECK-NEXT: [[STRUCT_STR_U4:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U4A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U4:@[0-9]+]] = private unnamed_addr constant [21 x i8] c"unsigned int a = %u\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U4:@[0-9]+]] = private unnamed_addr constant [25 x i8] c"unsigned int a = %u\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U4:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit5.a = private unnamed_addr constant %struct.U5A { i64 12 }, align 8
 // CHECK-NEXT: [[STRUCT_STR_U5:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U5A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U5:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"long a = %ld\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U5:@[0-9]+]] = private unnamed_addr constant [18 x i8] c"long a = %ld\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U5:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit6.a = private unnamed_addr constant %struct.U6A { i64 12 }, align 8
 // CHECK-NEXT: [[STRUCT_STR_U6:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U6A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U6:@[0-9]+]] = private unnamed_addr constant [23 x i8] c"unsigned long a = %lu\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U6:@[0-9]+]] = private unnamed_addr constant [27 x i8] c"unsigned long a = %lu\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U6:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit7.a = private unnamed_addr constant %struct.U7A { i64 12 }, align 8
 // CHECK-NEXT: [[STRUCT_STR_U7:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U7A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U7:@[0-9]+]] = private unnamed_addr constant [20 x i8] c"long long a = %lld\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U7:@[0-9]+]] = private unnamed_addr constant [24 x i8] c"long long a = %lld\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U7:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit8.a = private unnamed_addr constant %struct.U8A { i64 12 }, align 8
 // CHECK-NEXT: [[STRUCT_STR_U8:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U8A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U8:@[0-9]+]] = private unnamed_addr constant [29 x i8] c"unsigned long long a = %llu\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U8:@[0-9]+]] = private unnamed_addr constant [33 x i8] c"unsigned long long a = %llu\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U8:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 

[PATCH] D122662: [Clang][CodeGen] Add constant array support for __builtin_dump_sturct, and beautify dump format

[Wang Yihan via Phabricator via cfe-commits]

yihanaa created this revision.
yihanaa added reviewers: erichkeane, aaron.ballman.
yihanaa added projects: LLVM, clang.
Herald added a project: All.
yihanaa requested review of this revision.
Herald added a subscriber: cfe-commits.

1. Add constant array support for __builtin_dump_sturct, the style like lldb

struct:

  struct S {
int x;
int y : 4;
int : 0;
int b[2][2];
float f;
struct T {
  int i;
} t;
struct {
  int i;
} foo;
struct Bar {
int x;
const char *B;
  } bar[2];
  };

output:

  struct S {
  int x = 100
  int y : 4 = 1
  int : 0
  int[2][2] b = [
  [0] = [
  [0] = 1
  [1] = 2
  ]
  [1] = [
  [0] = 3
  [1] = 4
  ]
  ]
  float f = 0.00
  struct T {
  int i = 2022
  }
  struct S::(unnamed) {
  int i = 4096
  }
  struct Bar[2] bar = [
  [0] = {
  int x = 1024
  const char * B = This is struct Bar[0]
  }
  [1] = {
  int x = 2048
  const char * B = This is struct Bar[1]
  }
  ]
  }

2. beautify dump format, add indent.

for example:
struct:

  struct A {
int a;
struct B {
  int b;
  struct C {
struct D {
  int d;
  union E {
int x;
int y;
  } e;
} d;
int c;
  } c;
} b;
  };

Before:

  struct A {
  int a = 0
  struct B {
  int b = 0
  struct C {
  struct D {
  int d = 0
  union E {
  int x = 0
  int y = 0
  }
  }
  int c = 0
  }
  }
  }

After:

  struct A {
  int a = 0
  struct B {
  int b = 0
  struct C {
  struct D {
  int d = 0
  union E {
  int x = 0
  int y = 0
  }
  }
  int c = 0
  }
  }
  }

3. Remove anonymous tag locations, powered by 'PrintingPolicy'

struct:

  struct S {
int a;
struct /* Anonymous*/ {
  int x;
} b;
int c;
  };

Before:

  struct S {
  int a = 0
  struct S::(unnamed at ./builtin_dump_struct.c:20:3) {
  int x = 0
  }
  int c = 0
  }

After:

  struct S {
  int a = 0
  struct S::(unnamed) {
  int x = 0
  }
  int c = 0
  }


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D122662

Files:
  clang/docs/LanguageExtensions.rst
  clang/docs/ReleaseNotes.rst
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/test/CodeGen/dump-struct-builtin.c

Index: clang/test/CodeGen/dump-struct-builtin.c
===
--- clang/test/CodeGen/dump-struct-builtin.c
+++ clang/test/CodeGen/dump-struct-builtin.c
@@ -1,99 +1,116 @@
 // RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm %s -o - | FileCheck %s
 
-#include "Inputs/stdio.h"
-#include 
+typedef signed char int8_t;
+typedef unsigned char uint8_t;
+int printf(const char *, ...);
 
 // CHECK: @__const.unit1.a = private unnamed_addr constant %struct.U1A { i16 12 }, align 2
 // CHECK-NEXT: [[STRUCT_STR_U1:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U1A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U1:@[0-9]+]] = private unnamed_addr constant [15 x i8] c"short a = %hd\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U1:@[0-9]+]] = private unnamed_addr constant [19 x i8] c"short a = %hd\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U1:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit2.a = private unnamed_addr constant %struct.U2A { i16 12 }, align 2
 // CHECK-NEXT: [[STRUCT_STR_U2:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U2A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U2:@[0-9]+]] = private unnamed_addr constant [24 x i8] c"unsigned short a = %hu\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U2:@[0-9]+]] = private unnamed_addr constant [28 x i8] c"unsigned short a = %hu\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U2:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit3.a = private unnamed_addr constant %struct.U3A { i32 12 }, align 4
 // CHECK-NEXT: [[STRUCT_STR_U3:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U3A {\0A\00", align 1
-// CHECK-NEXT: [[FIELD_U3:@[0-9]+]] = private unnamed_addr constant [12 x i8] c"int a = %d\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U3:@[0-9]+]] = private unnamed_addr constant [16 x i8] c"int a = %d\0A\00", align 1
 // CHECK-NEXT: [[END_STRUCT_U3:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit4.a = private unnamed_addr constant %struct.U4A { i32 12 }, align 4
 // CHECK-NEXT: [[STRUCT_STR_U4:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U4A {\0A\00", align 1
-// 

[PATCH] D122248: [clang][CodeGen]Fix clang crash and add bitfield support in __builtin_dump_struct

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

In D122248#3406180 , @erichkeane 
wrote:

> In D122248#3406162 , @yihanaa wrote:
>
>> In D122248#3406145 , 
>> @aaron.ballman wrote:
>>
>>> LGTM aside from a tiny nit with one of the release notes.
>>
>> I'd be happy to fix it
>>
>> In D122248#3406145 , 
>> @aaron.ballman wrote:
>>
>>> LGTM aside from a tiny nit with one of the release notes.
>
> Don't worry about it, I'll make the change as part of committing.

Thanks, i edited it, if there is anything wrong, please help me correct it


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122248/new/

https://reviews.llvm.org/D122248

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122248: [clang][CodeGen]Fix clang crash and add bitfield support in __builtin_dump_struct

[Wang Yihan via Phabricator via cfe-commits]

yihanaa updated this revision to Diff 418008.
yihanaa added a comment.

Put the dump format changes under the "Non-comprehensive list of changes in 
this release" heading instead.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122248/new/

https://reviews.llvm.org/D122248

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/test/CodeGen/dump-struct-builtin.c

Index: clang/test/CodeGen/dump-struct-builtin.c
===
--- clang/test/CodeGen/dump-struct-builtin.c
+++ clang/test/CodeGen/dump-struct-builtin.c
@@ -4,114 +4,95 @@
 #include 
 
 // CHECK: @__const.unit1.a = private unnamed_addr constant %struct.U1A { i16 12 }, align 2
-// CHECK-NEXT: [[STRUCT_STR_U1:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U1A {\0A\00"
-// CHECK-NEXT: [[FIELD_U1:@[0-9]+]] = private unnamed_addr constant [11 x i8] c"short a : \00"
-// CHECK-NEXT: [[FORMAT_U1:@[0-9]+]] = private unnamed_addr constant [5 x i8] c"%hd\0A\00"
-// CHECK-NEXT: [[END_STRUCT_U1:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00"
+// CHECK-NEXT: [[STRUCT_STR_U1:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U1A {\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U1:@[0-9]+]] = private unnamed_addr constant [15 x i8] c"short a = %hd\0A\00", align 1
+// CHECK-NEXT: [[END_STRUCT_U1:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit2.a = private unnamed_addr constant %struct.U2A { i16 12 }, align 2
-// CHECK-NEXT: [[STRUCT_STR_U2:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U2A {\0A\00"
-// CHECK-NEXT: [[FIELD_U2:@[0-9]+]] = private unnamed_addr constant [20 x i8] c"unsigned short a : \00"
-// CHECK-NEXT: [[FORMAT_U2:@[0-9]+]] = private unnamed_addr constant [5 x i8] c"%hu\0A\00"
-// CHECK-NEXT: [[END_STRUCT_U2:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00"
+// CHECK-NEXT: [[STRUCT_STR_U2:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U2A {\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U2:@[0-9]+]] = private unnamed_addr constant [24 x i8] c"unsigned short a = %hu\0A\00", align 1
+// CHECK-NEXT: [[END_STRUCT_U2:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit3.a = private unnamed_addr constant %struct.U3A { i32 12 }, align 4
-// CHECK-NEXT: [[STRUCT_STR_U3:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U3A {\0A\00"
-// CHECK-NEXT: [[FIELD_U3:@[0-9]+]] = private unnamed_addr constant [9 x i8] c"int a : \00"
-// CHECK-NEXT: [[FORMAT_U3:@[0-9]+]] = private unnamed_addr constant [4 x i8] c"%d\0A\00"
-// CHECK-NEXT: [[END_STRUCT_U3:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00"
+// CHECK-NEXT: [[STRUCT_STR_U3:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U3A {\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U3:@[0-9]+]] = private unnamed_addr constant [12 x i8] c"int a = %d\0A\00", align 1
+// CHECK-NEXT: [[END_STRUCT_U3:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit4.a = private unnamed_addr constant %struct.U4A { i32 12 }, align 4
-// CHECK-NEXT: [[STRUCT_STR_U4:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U4A {\0A\00"
-// CHECK-NEXT: [[FIELD_U4:@[0-9]+]] = private unnamed_addr constant [18 x i8] c"unsigned int a : \00"
-// CHECK-NEXT: [[FORMAT_U4:@[0-9]+]] = private unnamed_addr constant [4 x i8] c"%u\0A\00"
-// CHECK-NEXT: [[END_STRUCT_U4:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00"
+// CHECK-NEXT: [[STRUCT_STR_U4:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U4A {\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U4:@[0-9]+]] = private unnamed_addr constant [21 x i8] c"unsigned int a = %u\0A\00", align 1
+// CHECK-NEXT: [[END_STRUCT_U4:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit5.a = private unnamed_addr constant %struct.U5A { i64 12 }, align 8
-// CHECK-NEXT: [[STRUCT_STR_U5:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U5A {\0A\00"
-// CHECK-NEXT: [[FIELD_U5:@[0-9]+]] = private unnamed_addr constant [10 x i8] c"long a : \00"
-// CHECK-NEXT: [[FORMAT_U5:@[0-9]+]] = private unnamed_addr constant [5 x i8] c"%ld\0A\00"
-// CHECK-NEXT: [[END_STRUCT_U5:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00"
+// CHECK-NEXT: [[STRUCT_STR_U5:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U5A {\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U5:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"long a = %ld\0A\00", align 1
+// CHECK-NEXT: [[END_STRUCT_U5:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit6.a = private unnamed_addr constant %struct.U6A { i64 12 }, align 8
-// CHECK-NEXT: [[STRUCT_STR_U6:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U6A {\0A\00"
-// CHECK-NEXT: [[FIELD_U6:@[0-9]+]] = private 

[PATCH] D122248: [clang][CodeGen]Fix clang crash and add bitfield support in __builtin_dump_struct

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

In D122248#3406145 , @aaron.ballman 
wrote:

> LGTM aside from a tiny nit with one of the release notes.

I'd be happy to fix it

In D122248#3406145 , @aaron.ballman 
wrote:

> LGTM aside from a tiny nit with one of the release notes.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122248/new/

https://reviews.llvm.org/D122248

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122248: [clang][CodeGen]Fix clang crash and add bitfield support in __builtin_dump_struct

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

In D122248#3406117 , @erichkeane 
wrote:

> LGTM!  Please give Aaron a few hours (perhaps until tomorrow?) to take 1 last 
> look before committing.
>
> Also, if you lack commit rights and need someone to commit for you, please 
> provide the name + email address you'd like it committed under.

Thanks @skan. I don’t have commit access, can you land this patch for me? (if 
there are no objections after a few days) Please use “wangyihan 
1135831...@qq.com” to commit the change.

In D122248#3406117 , @erichkeane 
wrote:

> LGTM!  Please give Aaron a few hours (perhaps until tomorrow?) to take 1 last 
> look before committing.
>
> Also, if you lack commit rights and need someone to commit for you, please 
> provide the name + email address you'd like it committed under.

Thanks for your help and review @erichkeane, @aaron.ballman  . I don’t have 
commit access, (if Aaron are no objections after a few hours) Please use 
“wangyihan 1135831...@qq.com” to commit the change.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122248/new/

https://reviews.llvm.org/D122248

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122248: [clang][CodeGen]Fix clang crash and add bitfield support in __builtin_dump_struct

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

Waitting for CI...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122248/new/

https://reviews.llvm.org/D122248

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122248: [clang][CodeGen]Fix clang crash and add bitfield support in __builtin_dump_struct

[Wang Yihan via Phabricator via cfe-commits]

yihanaa updated this revision to Diff 417999.
yihanaa marked an inline comment as done.
yihanaa added a comment.

Use ``FD->getDeclName().empty()`` instead of ``FD->getNameAsString().empty()``
Add a the format changes to release notes


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122248/new/

https://reviews.llvm.org/D122248

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/test/CodeGen/dump-struct-builtin.c

Index: clang/test/CodeGen/dump-struct-builtin.c
===
--- clang/test/CodeGen/dump-struct-builtin.c
+++ clang/test/CodeGen/dump-struct-builtin.c
@@ -4,114 +4,95 @@
 #include 
 
 // CHECK: @__const.unit1.a = private unnamed_addr constant %struct.U1A { i16 12 }, align 2
-// CHECK-NEXT: [[STRUCT_STR_U1:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U1A {\0A\00"
-// CHECK-NEXT: [[FIELD_U1:@[0-9]+]] = private unnamed_addr constant [11 x i8] c"short a : \00"
-// CHECK-NEXT: [[FORMAT_U1:@[0-9]+]] = private unnamed_addr constant [5 x i8] c"%hd\0A\00"
-// CHECK-NEXT: [[END_STRUCT_U1:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00"
+// CHECK-NEXT: [[STRUCT_STR_U1:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U1A {\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U1:@[0-9]+]] = private unnamed_addr constant [15 x i8] c"short a = %hd\0A\00", align 1
+// CHECK-NEXT: [[END_STRUCT_U1:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit2.a = private unnamed_addr constant %struct.U2A { i16 12 }, align 2
-// CHECK-NEXT: [[STRUCT_STR_U2:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U2A {\0A\00"
-// CHECK-NEXT: [[FIELD_U2:@[0-9]+]] = private unnamed_addr constant [20 x i8] c"unsigned short a : \00"
-// CHECK-NEXT: [[FORMAT_U2:@[0-9]+]] = private unnamed_addr constant [5 x i8] c"%hu\0A\00"
-// CHECK-NEXT: [[END_STRUCT_U2:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00"
+// CHECK-NEXT: [[STRUCT_STR_U2:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U2A {\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U2:@[0-9]+]] = private unnamed_addr constant [24 x i8] c"unsigned short a = %hu\0A\00", align 1
+// CHECK-NEXT: [[END_STRUCT_U2:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit3.a = private unnamed_addr constant %struct.U3A { i32 12 }, align 4
-// CHECK-NEXT: [[STRUCT_STR_U3:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U3A {\0A\00"
-// CHECK-NEXT: [[FIELD_U3:@[0-9]+]] = private unnamed_addr constant [9 x i8] c"int a : \00"
-// CHECK-NEXT: [[FORMAT_U3:@[0-9]+]] = private unnamed_addr constant [4 x i8] c"%d\0A\00"
-// CHECK-NEXT: [[END_STRUCT_U3:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00"
+// CHECK-NEXT: [[STRUCT_STR_U3:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U3A {\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U3:@[0-9]+]] = private unnamed_addr constant [12 x i8] c"int a = %d\0A\00", align 1
+// CHECK-NEXT: [[END_STRUCT_U3:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit4.a = private unnamed_addr constant %struct.U4A { i32 12 }, align 4
-// CHECK-NEXT: [[STRUCT_STR_U4:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U4A {\0A\00"
-// CHECK-NEXT: [[FIELD_U4:@[0-9]+]] = private unnamed_addr constant [18 x i8] c"unsigned int a : \00"
-// CHECK-NEXT: [[FORMAT_U4:@[0-9]+]] = private unnamed_addr constant [4 x i8] c"%u\0A\00"
-// CHECK-NEXT: [[END_STRUCT_U4:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00"
+// CHECK-NEXT: [[STRUCT_STR_U4:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U4A {\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U4:@[0-9]+]] = private unnamed_addr constant [21 x i8] c"unsigned int a = %u\0A\00", align 1
+// CHECK-NEXT: [[END_STRUCT_U4:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit5.a = private unnamed_addr constant %struct.U5A { i64 12 }, align 8
-// CHECK-NEXT: [[STRUCT_STR_U5:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U5A {\0A\00"
-// CHECK-NEXT: [[FIELD_U5:@[0-9]+]] = private unnamed_addr constant [10 x i8] c"long a : \00"
-// CHECK-NEXT: [[FORMAT_U5:@[0-9]+]] = private unnamed_addr constant [5 x i8] c"%ld\0A\00"
-// CHECK-NEXT: [[END_STRUCT_U5:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00"
+// CHECK-NEXT: [[STRUCT_STR_U5:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U5A {\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U5:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"long a = %ld\0A\00", align 1
+// CHECK-NEXT: [[END_STRUCT_U5:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit6.a = private unnamed_addr constant %struct.U6A { i64 12 }, align 8
-// CHECK-NEXT: [[STRUCT_STR_U6:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U6A 

[PATCH] D122248: [clang][CodeGen]Fix clang crash and add bitfield support in __builtin_dump_struct

[Wang Yihan via Phabricator via cfe-commits]

yihanaa marked an inline comment as done.
yihanaa added a comment.

Maybe add the changes under "Attribute Changes in Clang"?




Comment at: clang/docs/ReleaseNotes.rst:81
   `Issue 50541 `_.
-
+- The builtin function __builtin_dump_struct would crash clang when the target 
+  struct have bitfield. Now it fixed, and __builtin_dump_struct support dump

erichkeane wrote:
> Perhaps worth making a separate release note for the change in format?  
Okay, Under which ReleaseNotes tag should I add? i can't think of a good idea 



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2093
+  // between type-name and ':'
+  if (!FD->getNameAsString().empty())
+Format += ' ';

erichkeane wrote:
> This seems like a pretty expensive way to do this... Can you do this as 
> `FD->getDeclName().empty()`?  
Okay


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122248/new/

https://reviews.llvm.org/D122248

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122248: [clang][CodeGen]Fix clang crash and add bitfield support in __builtin_dump_struct

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

In D122248#3405866 , @aaron.ballman 
wrote:

> In D122248#3405644 , @yihanaa wrote:
>
>> In D122248#3405166 , @erichkeane 
>> wrote:
>>
>>> In D122248#3405062 , 
>>> @aaron.ballman wrote:
>>>
 In D122248#3403734 , @yihanaa 
 wrote:

> What if we don't emit '=' for zero-width bitfield, like this:
>
>   struct Bar {
>   unsigned c : 1;
>   unsigned : 3;
>   unsigned : 0;
>   unsigned b;
>   };
>   
>   struct Bar {
>   unsigned int c : 1 = 0
>   unsigned int   : 3 = 0
>   unsigned int   : 0
>   unsigned int b = 0
>   }
>
> What do you all think?

 I like this idea best of all!
>>>
>>> Agreed!
>>
>> @erichkeane @aaron.ballman 
>> Previously we used FieldDecl->getNameAsString to get the field name, but the 
>> comments for this function indicate that it is Deprecated，and suggestion 
>> move clients to getName().
>>
>> The FieldDecl->getName() return the anonymous inner struct's name like:
>>
>>   struct T3A {
>> union {
>>   int a;
>>   char b[4];
>> };
>>   };
>>
>>   struct T3A {
>>   union T3A::(anonymous at ./builtin_dump_struct.c:77:5) {
>>   int a = 42
>>   char[4] b = 0x2a
>>   }
>>   }
>>
>> what do you all think?
>
> I would probably keep using `getNameAsString()` as I don't think the extra 
> information helps overly much. That interface was marked deprecated 13 years 
> ago, so don't feel bad for making use of it.

Okay,

In D122248#3405926 , @erichkeane 
wrote:

> Agreed.  `DeclarationName::getAsString` is in no way marked deprecated 
> though, so you could call that?
>
> `FieldDecl->getDeclName()->getAsString()` or the `DeclarationName` 
> `operator<<` would both be equivalent with no threat of deprecation.

Thanks @erichkeane @aaron.ballman , i have updated the diff.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122248/new/

https://reviews.llvm.org/D122248

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122248: [clang][CodeGen]Fix clang crash and add bitfield support in __builtin_dump_struct

[Wang Yihan via Phabricator via cfe-commits]

yihanaa updated this revision to Diff 417990.
yihanaa added a comment.

Support dump bitwidth of bitfields, and unnamed bitfields.
for example:

  struct Bar {
  unsigned c : 1;
  unsigned : 3;
  unsigned : 0;
  unsigned b;
  };
  
  struct Bar {
  unsigned int c : 1 = 0
  unsigned int   : 3 = 0
  unsigned int   : 0
  unsigned int b = 0
  }


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122248/new/

https://reviews.llvm.org/D122248

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/test/CodeGen/dump-struct-builtin.c

Index: clang/test/CodeGen/dump-struct-builtin.c
===
--- clang/test/CodeGen/dump-struct-builtin.c
+++ clang/test/CodeGen/dump-struct-builtin.c
@@ -4,114 +4,95 @@
 #include 
 
 // CHECK: @__const.unit1.a = private unnamed_addr constant %struct.U1A { i16 12 }, align 2
-// CHECK-NEXT: [[STRUCT_STR_U1:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U1A {\0A\00"
-// CHECK-NEXT: [[FIELD_U1:@[0-9]+]] = private unnamed_addr constant [11 x i8] c"short a : \00"
-// CHECK-NEXT: [[FORMAT_U1:@[0-9]+]] = private unnamed_addr constant [5 x i8] c"%hd\0A\00"
-// CHECK-NEXT: [[END_STRUCT_U1:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00"
+// CHECK-NEXT: [[STRUCT_STR_U1:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U1A {\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U1:@[0-9]+]] = private unnamed_addr constant [15 x i8] c"short a = %hd\0A\00", align 1
+// CHECK-NEXT: [[END_STRUCT_U1:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit2.a = private unnamed_addr constant %struct.U2A { i16 12 }, align 2
-// CHECK-NEXT: [[STRUCT_STR_U2:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U2A {\0A\00"
-// CHECK-NEXT: [[FIELD_U2:@[0-9]+]] = private unnamed_addr constant [20 x i8] c"unsigned short a : \00"
-// CHECK-NEXT: [[FORMAT_U2:@[0-9]+]] = private unnamed_addr constant [5 x i8] c"%hu\0A\00"
-// CHECK-NEXT: [[END_STRUCT_U2:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00"
+// CHECK-NEXT: [[STRUCT_STR_U2:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U2A {\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U2:@[0-9]+]] = private unnamed_addr constant [24 x i8] c"unsigned short a = %hu\0A\00", align 1
+// CHECK-NEXT: [[END_STRUCT_U2:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit3.a = private unnamed_addr constant %struct.U3A { i32 12 }, align 4
-// CHECK-NEXT: [[STRUCT_STR_U3:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U3A {\0A\00"
-// CHECK-NEXT: [[FIELD_U3:@[0-9]+]] = private unnamed_addr constant [9 x i8] c"int a : \00"
-// CHECK-NEXT: [[FORMAT_U3:@[0-9]+]] = private unnamed_addr constant [4 x i8] c"%d\0A\00"
-// CHECK-NEXT: [[END_STRUCT_U3:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00"
+// CHECK-NEXT: [[STRUCT_STR_U3:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U3A {\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U3:@[0-9]+]] = private unnamed_addr constant [12 x i8] c"int a = %d\0A\00", align 1
+// CHECK-NEXT: [[END_STRUCT_U3:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit4.a = private unnamed_addr constant %struct.U4A { i32 12 }, align 4
-// CHECK-NEXT: [[STRUCT_STR_U4:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U4A {\0A\00"
-// CHECK-NEXT: [[FIELD_U4:@[0-9]+]] = private unnamed_addr constant [18 x i8] c"unsigned int a : \00"
-// CHECK-NEXT: [[FORMAT_U4:@[0-9]+]] = private unnamed_addr constant [4 x i8] c"%u\0A\00"
-// CHECK-NEXT: [[END_STRUCT_U4:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00"
+// CHECK-NEXT: [[STRUCT_STR_U4:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U4A {\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U4:@[0-9]+]] = private unnamed_addr constant [21 x i8] c"unsigned int a = %u\0A\00", align 1
+// CHECK-NEXT: [[END_STRUCT_U4:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit5.a = private unnamed_addr constant %struct.U5A { i64 12 }, align 8
-// CHECK-NEXT: [[STRUCT_STR_U5:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U5A {\0A\00"
-// CHECK-NEXT: [[FIELD_U5:@[0-9]+]] = private unnamed_addr constant [10 x i8] c"long a : \00"
-// CHECK-NEXT: [[FORMAT_U5:@[0-9]+]] = private unnamed_addr constant [5 x i8] c"%ld\0A\00"
-// CHECK-NEXT: [[END_STRUCT_U5:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00"
+// CHECK-NEXT: [[STRUCT_STR_U5:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"struct U5A {\0A\00", align 1
+// CHECK-NEXT: [[FIELD_U5:@[0-9]+]] = private unnamed_addr constant [14 x i8] c"long a = %ld\0A\00", align 1
+// CHECK-NEXT: [[END_STRUCT_U5:@[0-9]+]] = private unnamed_addr constant [3 x i8] c"}\0A\00", align 1
 
 // CHECK: @__const.unit6.a = private unnamed_addr constant %struct.U6A { i64 12 }, 

[PATCH] D122248: [clang][CodeGen]Fix clang crash and add bitfield support in __builtin_dump_struct

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

In D122248#3405166 , @erichkeane 
wrote:

> In D122248#3405062 , @aaron.ballman 
> wrote:
>
>> In D122248#3403734 , @yihanaa 
>> wrote:
>>
>>> What if we don't emit '=' for zero-width bitfield, like this:
>>>
>>>   struct Bar {
>>>   unsigned c : 1;
>>>   unsigned : 3;
>>>   unsigned : 0;
>>>   unsigned b;
>>>   };
>>>   
>>>   struct Bar {
>>>   unsigned int c : 1 = 0
>>>   unsigned int   : 3 = 0
>>>   unsigned int   : 0
>>>   unsigned int b = 0
>>>   }
>>>
>>> What do you all think?
>>
>> I like this idea best of all!
>
> Agreed!

@erichkeane @aaron.ballman


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122248/new/

https://reviews.llvm.org/D122248

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits