[PATCH] D116774: AST: Move __va_list tag to the top level on ARM architectures.

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

Ping? I'd really like to get this fixed in 14.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116774

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

[PATCH] D116774: AST: Move __va_list tag to the top level on ARM architectures.

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

Ping?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116774

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

[PATCH] D116774: AST: Move __va_list tag to the top level on ARM architectures.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D116774#3227280 , @jrtc27 wrote:

> The fact that va_list is in the std namespace does leak out into 
> __builtin_dump_struct, possibly the odd other place, and of course to 
> external AST consumers.

It can also leak out in funny places like AST dumping (to text, but more 
interestingly, to JSON). But the one place that may be observable and really 
matters (that I can think of) are AST matchers that get used by clang-tidy.

> I think it'd make sense to keep ASTContext as putting it in the std namespace 
> for C++ (like it does for Arm, and used to for AArch64) but also have 
> ItaniumMangle override it to the std namespace so that non-C++ gets the right 
> mangling? Otherwise the AST and __builtin_dump_struct won't match the Arm 
> spec.

I think this could make some sense. We typically try to have the AST reflect 
the reality of the program, and from that perspective, I think I would expect 
there to be a `std` namespace component for this in the AST if the spec calls 
for one even when obtaining the type through `stdarg.h` instead of `cstdarg`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116774

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

[PATCH] D116774: AST: Move __va_list tag to the top level on ARM architectures.

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

I'm not aware of any of those places causing an actual problem though? The AST 
isn't a stable interface, and __builtin_dump_struct is for debugging purposes 
only. I would expect debug info consumers to be able to handle __va_list in the 
global namespace as this is the status quo for C.

So I'm somewhat inclined to do the simple thing here first, and then look at 
making things more conditional if a problem comes up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116774

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

[PATCH] D116774: AST: Move __va_list tag to the top level on ARM architectures.

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

The fact that va_list is in the std namespace does leak out into 
__builtin_dump_struct, possibly the odd other place, and of course to AST 
consumers.

I think it'd make sense to keep ASTContext as putting it in the std namespace 
for C++ (like it does for Arm, and used to for AArch64) but also have 
ItaniumMangle override it to the std namespace so that non-C++ gets the right 
mangling?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116774

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

[PATCH] D116774: AST: Move __va_list tag to the top level on ARM architectures.

[Peter Collingbourne via Phabricator via cfe-commits]

pcc created this revision.
pcc added reviewers: rsmith, eugenis, jrtc27.
Herald added a subscriber: kristof.beyls.
pcc requested review of this revision.
Herald added a project: clang.

In post-commit feedback on D104830  Jessica 
Clarke pointed out that
unconditionally adding __va_list to the std namespace caused namespace
debug info to be emitted in C, which is not only inappropriate but
turned out to confuse the dtrace tool. Therefore, move __va_list to the
top level unconditionally so that the correct debug info is generated.

To avoid breaking name mangling for __va_list, teach the Itanium name
mangler to mangle it as if it were in the std namespace when targeting
ARM architectures. This logic is not needed for the Microsoft name
mangler because Microsoft platforms define va_list as a typedef of
char *.

It was also noted that 32-bit ARM has the same issue as was fixed
for 64-bit ARM in D104830 , so do the same 
for that architecture.

Depends on D116773 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D116774

Files:
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/test/CodeGen/aarch64-varargs.c
  clang/test/CodeGen/arm64-be-hfa-vararg.c
  clang/test/Headers/stdarg.cpp

Index: clang/test/Headers/stdarg.cpp
===
--- clang/test/Headers/stdarg.cpp
+++ clang/test/Headers/stdarg.cpp
@@ -15,8 +15,8 @@
 
 #include 
 
-// AARCH64-C: define {{.*}} @f(i32 %n, %"struct.std::__va_list"* %list)
-// AARCH64-CXX: define {{.*}} @_Z1fiSt9__va_list(i32 %n, %"struct.std::__va_list"* %list)
+// AARCH64-C: define {{.*}} @f(i32 %n, %struct.__va_list* %list)
+// AARCH64-CXX: define {{.*}} @_Z1fiSt9__va_list(i32 %n, %struct.__va_list* %list)
 // X86_64-C: define {{.*}} @f(i32 %n, %struct.__va_list_tag* %list)
 // X86_64-CXX: define {{.*}} @_Z1fiP13__va_list_tag(i32 %n, %struct.__va_list_tag* %list)
 // PPC64-C: define {{.*}} @f(i32 signext %n, i8* %list)
Index: clang/test/CodeGen/arm64-be-hfa-vararg.c
===
--- clang/test/CodeGen/arm64-be-hfa-vararg.c
+++ clang/test/CodeGen/arm64-be-hfa-vararg.c
@@ -4,12 +4,12 @@
 
 // A single member HFA must be aligned just like a non-HFA register argument.
 double callee(int a, ...) {
-// CHECK: [[REGPP:%.*]] = getelementptr inbounds %"struct.std::__va_list", %"struct.std::__va_list"* [[VA:%.*]], i32 0, i32 2
+// CHECK: [[REGPP:%.*]] = getelementptr inbounds %struct.__va_list, %struct.__va_list* [[VA:%.*]], i32 0, i32 2
 // CHECK: [[REGP:%.*]] = load i8*, i8** [[REGPP]], align 8
 // CHECK: [[OFFSET0:%.*]] = getelementptr inbounds i8, i8* [[REGP]], i32 {{.*}}
 // CHECK: [[OFFSET1:%.*]] = getelementptr inbounds i8, i8* [[OFFSET0]], i64 8
 
-// CHECK: [[MEMPP:%.*]] = getelementptr inbounds %"struct.std::__va_list", %"struct.std::__va_list"* [[VA:%.*]], i32 0, i32 0
+// CHECK: [[MEMPP:%.*]] = getelementptr inbounds %struct.__va_list, %struct.__va_list* [[VA:%.*]], i32 0, i32 0
 // CHECK: [[MEMP:%.*]] = load i8*, i8** [[MEMPP]], align 8
 // CHECK: [[NEXTP:%.*]] = getelementptr inbounds i8, i8* [[MEMP]], i64 8
 // CHECK: store i8* [[NEXTP]], i8** [[MEMPP]], align 8
Index: clang/test/CodeGen/aarch64-varargs.c
===
--- clang/test/CodeGen/aarch64-varargs.c
+++ clang/test/CodeGen/aarch64-varargs.c
@@ -11,18 +11,18 @@
 int simple_int(void) {
 // CHECK-LABEL: define{{.*}} i32 @simple_int
   return va_arg(the_list, int);
-// CHECK: [[GR_OFFS:%[a-z_0-9]+]] = load i32, i32* getelementptr inbounds (%"struct.std::__va_list", %"struct.std::__va_list"* @the_list, i32 0, i32 3)
+// CHECK: [[GR_OFFS:%[a-z_0-9]+]] = load i32, i32* getelementptr inbounds (%struct.__va_list, %struct.__va_list* @the_list, i32 0, i32 3)
 // CHECK: [[EARLY_ONSTACK:%[a-z_0-9]+]] = icmp sge i32 [[GR_OFFS]], 0
 // CHECK: br i1 [[EARLY_ONSTACK]], label %[[VAARG_ON_STACK:[a-z_.0-9]+]], label %[[VAARG_MAYBE_REG:[a-z_.0-9]+]]
 
 // CHECK: [[VAARG_MAYBE_REG]]
 // CHECK: [[NEW_REG_OFFS:%[a-z_0-9]+]] = add i32 [[GR_OFFS]], 8
-// CHECK: store i32 [[NEW_REG_OFFS]], i32* getelementptr inbounds (%"struct.std::__va_list", %"struct.std::__va_list"* @the_list, i32 0, i32 3)
+// CHECK: store i32 [[NEW_REG_OFFS]], i32* getelementptr inbounds (%struct.__va_list, %struct.__va_list* @the_list, i32 0, i32 3)
 // CHECK: [[INREG:%[a-z_0-9]+]] = icmp sle i32 [[NEW_REG_OFFS]], 0
 // CHECK: br i1 [[INREG]], label %[[VAARG_IN_REG:[a-z_.0-9]+]], label %[[VAARG_ON_STACK]]
 
 // CHECK: [[VAARG_IN_REG]]
-// CHECK: [[REG_TOP:%[a-z_0-9]+]] = load i8*, i8** getelementptr inbounds (%"struct.std::__va_list", %"struct.std::__va_list"* @the_list, i32 0, i32 1)
+// CHECK: [[REG_TOP:%[a-z_0-9]+]] = load i8*, i8** getelementptr inbounds (%struct.__va_list, %struct.__va_list* @the_list, i32 0, i32 1)
 // CHECK: [[REG_ADDR:%[a-z_0-9]+]] =