[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[Bruno De Fraine via Phabricator via cfe-commits]

brunodf added inline comments.



Comment at: lib/Sema/SemaType.cpp:4877
   T = Context.getFunctionType(T, ParamTys, EPI);
   T = state.getSema().Context.getAddrSpaceQualType(T, AS);
 } else {

I follow all of the above (from the point "a class member function has an 
address space"), but then I take issue with this line (already from Mikael).

You look at the declaration's attributes, to derive the ASIdx relating to the 
method's this argument. You mark the relevant attributes as invalid, to prevent 
them from being considered in "processTypeAttrs" after the switch that we break 
below. The ASIdx is stored in the qualifiers EPI to go to the FunctionProtoType 
(this will affect getThisType). This all seems fine.

But then this line adds the address space qualification to T, the type of the 
declared function itself. This seems unnecessary, and conceptually wrong: while 
the function may have an address space in syntax, this address space applies to 
the this argument, not to the function object (and a pointer to the function is 
not a pointer to this address space etc.). In short, I think this line should 
be removed.


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

https://reviews.llvm.org/D55850



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

[PATCH] D59988: [PR41276] Generate address space cast of 'this' for objects attributed by an address space in C++

[Bruno De Fraine via Phabricator via cfe-commits]

brunodf added inline comments.



Comment at: lib/CodeGen/CGClass.cpp:2025
+ThisPtr =
+Builder.CreatePointerBitCastOrAddrSpaceCast(This.getPointer(), 
NewType);
   }

rjmccall wrote:
> Anastasia wrote:
> > I am a bit unsure if `performAddrSpaceCast` should be used, but considering 
> > that we know that we are not casting a constant it should be fine?
> > 
> > If not any suggestions how to propagate `LangAS` of 'this' here. Some 
> > thoughts I have are:
> > - Try to list the conversion up in the call stack
> > - Pass `LangAS` all the way to here
> I feel like `This` should just be in the right address space for the 
> constructor at the point `EmitCXXConstructorCall` is called.  We don't expect 
> this function to do any other semantic conversions.  Or is this necessary to 
> handle special-case use of things like trivial default / copy constructors?
Where could the conversion of `this` be listed in the clang AST? `this` seems 
implicit there.

Passing along `LangAS` seems to have some precedent. `EmitCXXConstructExpr` 
(which calls `EmitCXXConstructorCall`) works on `AggValueSlot` which carries 
the original qualifiers. Currently not yet used for address space (but this 
seems similar to me):

```
  /// \param quals - The qualifiers that dictate how the slot should
  /// be initialied. Only 'volatile' and the Objective-C lifetime
  /// qualifiers matter.
```



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

https://reviews.llvm.org/D59988



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

[PATCH] D150930: [Driver] Accept and ignore -fno-lifetime-dse argument

[Bruno De Fraine via Phabricator via cfe-commits]

brunodf added a comment.

Hi,

I found this review request and I just want to comment that I find it strange 
that it was rejected.

@MaskRay I understand that using a compile_commands.json configured for gcc 
with clang-based tools is not a supported use case, but still the clang driver 
was explicitly designed to emulate gcc: "The clang tool is the compiler driver 
and front-end, which is designed to be a drop-in replacement for the gcc 
command" and "The 'clang' driver is designed to work as closely to GCC as 
possible to maximize portability." are quotes from 
https://clang.llvm.org/get_started.html
In that regard, `clang_ignored_gcc_optimization_f_Group` is logical and it 
includes many options that you cite like `-falign-jumps=`, `-falign-loops=`, 
`-fmerge-constants`, `-fschedule-insns`, etc.
Sure, more projects support clang directly now, but I was not aware there is a 
change in this policy, or that there is a "stop" on adding more options (in 
that case, it would be consistent that the documentation is adapted to say that 
clang is only drop-in compatible with some historic version of gcc).

In my view, the main objection to "accept and ignore" a GCC option is when the 
option provides some guarantees that clang/LLVM cannot uphold. For example, 
ignoring `-fno-strict-aliasing` would be dangerous if you actually carry out 
type-based aliasing optimizations, because programs that compile with it likely 
contain violations of the strict aliasing rules. It seems that 
`-fno-lifetime-dse` similarly intends to allow violating a language rule. I'm 
not aware if clang/LLVM contains optimization that exploit this language rule 
(since the option appears in the context of the LLVM code base itself, and 
because compiling this code base with clang itself is well tested in many 
configurations, I suspect not), but if it does (now or in the future), ignoring 
this option is dangerous.

Regards,
Bruno


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150930

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

[PATCH] D126956: [tbaa] Handle base classes in struct tbaa

[Bruno De Fraine via Phabricator via cfe-commits]

brunodf updated this revision to Diff 441017.
brunodf added a comment.
Herald added a subscriber: mgrang.

New version that fixes the problem with base classes not appearing in order of 
increasing offset.

This issue was discovered in the multistage build, but the test case has now 
been extended for this situation.

Also changed to obtain the the size of the base subobject with `getDataSize()` 
since other subobjects may be allocated in its tail padding.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126956

Files:
  clang/lib/CodeGen/CodeGenTBAA.cpp
  clang/test/CodeGen/tbaa-class.cpp
  clang/unittests/CodeGen/TBAAMetadataTest.cpp

Index: clang/unittests/CodeGen/TBAAMetadataTest.cpp
===
--- clang/unittests/CodeGen/TBAAMetadataTest.cpp
+++ clang/unittests/CodeGen/TBAAMetadataTest.cpp
@@ -968,13 +968,10 @@
   MConstInt(0)),
 MConstInt(0));
 
-  auto ClassDerived = MMTuple(
-MMString("_ZTS7Derived"),
-MMTuple(
-  MMString("short"),
-  OmnipotentCharCXX,
-  MConstInt(0)),
-MConstInt(4));
+  auto ClassDerived =
+  MMTuple(MMString("_ZTS7Derived"), ClassBase, MConstInt(0),
+  MMTuple(MMString("short"), OmnipotentCharCXX, MConstInt(0)),
+  MConstInt(4));
 
   const Instruction *I = match(BB,
   MInstruction(Instruction::Store,
@@ -1047,13 +1044,10 @@
   MConstInt(0)),
 MConstInt(Compiler.PtrSize));
 
-  auto ClassDerived = MMTuple(
-MMString("_ZTS7Derived"),
-MMTuple(
-  MMString("short"),
-  OmnipotentCharCXX,
-  MConstInt(0)),
-MConstInt(Compiler.PtrSize + 4));
+  auto ClassDerived =
+  MMTuple(MMString("_ZTS7Derived"), ClassBase, MConstInt(0),
+  MMTuple(MMString("short"), OmnipotentCharCXX, MConstInt(0)),
+  MConstInt(Compiler.PtrSize + 4));
 
   const Instruction *I = match(BB,
   MInstruction(Instruction::Store,
Index: clang/test/CodeGen/tbaa-class.cpp
===
--- clang/test/CodeGen/tbaa-class.cpp
+++ clang/test/CodeGen/tbaa-class.cpp
@@ -51,6 +51,25 @@
uint32_t f32_2;
 };
 
+class StructT {
+public:
+  uint32_t f32_2;
+  void foo();
+};
+class StructM1 : public StructS, public StructT {
+public:
+  uint16_t f16_2;
+};
+class StructDyn {
+public:
+  uint32_t f32_2;
+  virtual void foo();
+};
+class StructM2 : public StructS, public StructDyn {
+public:
+  uint16_t f16_2;
+};
+
 uint32_t g(uint32_t *s, StructA *A, uint64_t count) {
 // CHECK-LABEL: define{{.*}} i32 @_Z1g
 // CHECK: store i32 1, i32* %{{.*}}, align 4, !tbaa [[TAG_i32:!.*]]
@@ -199,6 +218,30 @@
   return b1->a.f32;
 }
 
+uint32_t g13(StructM1 *M, StructS *S) {
+  // CHECK-LABEL: define{{.*}} i32 @_Z3g13
+  // CHECK: store i16 1, i16* %{{.*}}, align 4, !tbaa [[TAG_i16]]
+  // CHECK: store i16 4, i16* %{{.*}}, align 4, !tbaa [[TAG_i16]]
+  // PATH-LABEL: define{{.*}} i32 @_Z3g13
+  // PATH: store i16 1, i16* %{{.*}}, align 4, !tbaa [[TAG_S_f16]]
+  // PATH: store i16 4, i16* %{{.*}}, align 4, !tbaa [[TAG_M1_f16_2:!.*]]
+  S->f16 = 1;
+  M->f16_2 = 4;
+  return S->f16;
+}
+
+uint32_t g14(StructM2 *M, StructS *S) {
+  // CHECK-LABEL: define{{.*}} i32 @_Z3g14
+  // CHECK: store i16 1, i16* %{{.*}}, align 4, !tbaa [[TAG_i16]]
+  // CHECK: store i16 4, i16* %{{.*}}, align 4, !tbaa [[TAG_i16]]
+  // PATH-LABEL: define{{.*}} i32 @_Z3g14
+  // PATH: store i16 1, i16* %{{.*}}, align 4, !tbaa [[TAG_S_f16]]
+  // PATH: store i16 4, i16* %{{.*}}, align 4, !tbaa [[TAG_M2_f16_2:!.*]]
+  S->f16 = 1;
+  M->f16_2 = 4;
+  return S->f16;
+}
+
 // CHECK: [[TYPE_char:!.*]] = !{!"omnipotent char", [[TAG_cxx_tbaa:!.*]],
 // CHECK: [[TAG_cxx_tbaa]] = !{!"Simple C++ TBAA"}
 // CHECK: [[TAG_i32]] = !{[[TYPE_i32:!.*]], [[TYPE_i32]], i64 0}
@@ -222,11 +265,17 @@
 // OLD-PATH: [[TYPE_S]] = !{!"_ZTS7StructS", [[TYPE_SHORT]], i64 0, [[TYPE_INT]], i64 4}
 // OLD-PATH: [[TAG_S_f16]] = !{[[TYPE_S]], [[TYPE_SHORT]], i64 0}
 // OLD-PATH: [[TAG_S2_f32_2]] = !{[[TYPE_S2:!.*]], [[TYPE_INT]], i64 12}
-// OLD-PATH: [[TYPE_S2]] = !{!"_ZTS8StructS2", [[TYPE_SHORT]], i64 8, [[TYPE_INT]], i64 12}
+// OLD-PATH: [[TYPE_S2]] = !{!"_ZTS8StructS2", [[TYPE_S]], i64 0, [[TYPE_SHORT]], i64 8, [[TYPE_INT]], i64 12}
 // OLD-PATH: [[TAG_C_b_a_f32]] = !{[[TYPE_C:!.*]], [[TYPE_INT]], i64 12}
 // OLD-PATH: [[TYPE_C]] = !{!"_ZTS7StructC", [[TYPE_SHORT]], i64 0, [[TYPE_B]], i64 4, [[TYPE_INT]], i64 28}
 // OLD-PATH: [[TAG_D_b_a_f32]] = !{[[TYPE_D:!.*]], [[TYPE_INT]], i64 12}
 // OLD-PATH: [[TYPE_D]] = !{!"_ZTS7StructD", [[TYPE_SHORT]], i64 0, [[TYPE_B]], i64 4, [[TYPE_INT]], i64 28, [[TYPE_CHAR]], i64 32}
+// OLD-PATH: [[TAG_M1_f16_2]] = !{[[TYPE_M1:!.*]], [[TYPE_SHORT]], i64 12}
+// OLD-PATH: [[TYPE_M1]] = !{!"_ZTS8StructM1", [[TYPE_S]], i64 0, [[TYPE_T:!.*]], i64 8, [[TYPE_SHORT]], i64 12}
+// OLD_PATH: [[TYPE_T]] = !{!"_ZTS7StructT", [[TYPE_INT]], i64 0}
+// OLD-

[PATCH] D126956: [tbaa] Handle base classes in struct tbaa

[Bruno De Fraine via Phabricator via cfe-commits]

brunodf updated this revision to Diff 441294.
brunodf added a comment.

Adding comment regarding empty subobjects.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126956

Files:
  clang/lib/CodeGen/CodeGenTBAA.cpp
  clang/test/CodeGen/tbaa-class.cpp
  clang/unittests/CodeGen/TBAAMetadataTest.cpp

Index: clang/unittests/CodeGen/TBAAMetadataTest.cpp
===
--- clang/unittests/CodeGen/TBAAMetadataTest.cpp
+++ clang/unittests/CodeGen/TBAAMetadataTest.cpp
@@ -968,13 +968,10 @@
   MConstInt(0)),
 MConstInt(0));
 
-  auto ClassDerived = MMTuple(
-MMString("_ZTS7Derived"),
-MMTuple(
-  MMString("short"),
-  OmnipotentCharCXX,
-  MConstInt(0)),
-MConstInt(4));
+  auto ClassDerived =
+  MMTuple(MMString("_ZTS7Derived"), ClassBase, MConstInt(0),
+  MMTuple(MMString("short"), OmnipotentCharCXX, MConstInt(0)),
+  MConstInt(4));
 
   const Instruction *I = match(BB,
   MInstruction(Instruction::Store,
@@ -1047,13 +1044,10 @@
   MConstInt(0)),
 MConstInt(Compiler.PtrSize));
 
-  auto ClassDerived = MMTuple(
-MMString("_ZTS7Derived"),
-MMTuple(
-  MMString("short"),
-  OmnipotentCharCXX,
-  MConstInt(0)),
-MConstInt(Compiler.PtrSize + 4));
+  auto ClassDerived =
+  MMTuple(MMString("_ZTS7Derived"), ClassBase, MConstInt(0),
+  MMTuple(MMString("short"), OmnipotentCharCXX, MConstInt(0)),
+  MConstInt(Compiler.PtrSize + 4));
 
   const Instruction *I = match(BB,
   MInstruction(Instruction::Store,
Index: clang/test/CodeGen/tbaa-class.cpp
===
--- clang/test/CodeGen/tbaa-class.cpp
+++ clang/test/CodeGen/tbaa-class.cpp
@@ -51,6 +51,25 @@
uint32_t f32_2;
 };
 
+class StructT {
+public:
+  uint32_t f32_2;
+  void foo();
+};
+class StructM1 : public StructS, public StructT {
+public:
+  uint16_t f16_2;
+};
+class StructDyn {
+public:
+  uint32_t f32_2;
+  virtual void foo();
+};
+class StructM2 : public StructS, public StructDyn {
+public:
+  uint16_t f16_2;
+};
+
 uint32_t g(uint32_t *s, StructA *A, uint64_t count) {
 // CHECK-LABEL: define{{.*}} i32 @_Z1g
 // CHECK: store i32 1, i32* %{{.*}}, align 4, !tbaa [[TAG_i32:!.*]]
@@ -199,6 +218,30 @@
   return b1->a.f32;
 }
 
+uint32_t g13(StructM1 *M, StructS *S) {
+  // CHECK-LABEL: define{{.*}} i32 @_Z3g13
+  // CHECK: store i16 1, i16* %{{.*}}, align 4, !tbaa [[TAG_i16]]
+  // CHECK: store i16 4, i16* %{{.*}}, align 4, !tbaa [[TAG_i16]]
+  // PATH-LABEL: define{{.*}} i32 @_Z3g13
+  // PATH: store i16 1, i16* %{{.*}}, align 4, !tbaa [[TAG_S_f16]]
+  // PATH: store i16 4, i16* %{{.*}}, align 4, !tbaa [[TAG_M1_f16_2:!.*]]
+  S->f16 = 1;
+  M->f16_2 = 4;
+  return S->f16;
+}
+
+uint32_t g14(StructM2 *M, StructS *S) {
+  // CHECK-LABEL: define{{.*}} i32 @_Z3g14
+  // CHECK: store i16 1, i16* %{{.*}}, align 4, !tbaa [[TAG_i16]]
+  // CHECK: store i16 4, i16* %{{.*}}, align 4, !tbaa [[TAG_i16]]
+  // PATH-LABEL: define{{.*}} i32 @_Z3g14
+  // PATH: store i16 1, i16* %{{.*}}, align 4, !tbaa [[TAG_S_f16]]
+  // PATH: store i16 4, i16* %{{.*}}, align 4, !tbaa [[TAG_M2_f16_2:!.*]]
+  S->f16 = 1;
+  M->f16_2 = 4;
+  return S->f16;
+}
+
 // CHECK: [[TYPE_char:!.*]] = !{!"omnipotent char", [[TAG_cxx_tbaa:!.*]],
 // CHECK: [[TAG_cxx_tbaa]] = !{!"Simple C++ TBAA"}
 // CHECK: [[TAG_i32]] = !{[[TYPE_i32:!.*]], [[TYPE_i32]], i64 0}
@@ -222,11 +265,17 @@
 // OLD-PATH: [[TYPE_S]] = !{!"_ZTS7StructS", [[TYPE_SHORT]], i64 0, [[TYPE_INT]], i64 4}
 // OLD-PATH: [[TAG_S_f16]] = !{[[TYPE_S]], [[TYPE_SHORT]], i64 0}
 // OLD-PATH: [[TAG_S2_f32_2]] = !{[[TYPE_S2:!.*]], [[TYPE_INT]], i64 12}
-// OLD-PATH: [[TYPE_S2]] = !{!"_ZTS8StructS2", [[TYPE_SHORT]], i64 8, [[TYPE_INT]], i64 12}
+// OLD-PATH: [[TYPE_S2]] = !{!"_ZTS8StructS2", [[TYPE_S]], i64 0, [[TYPE_SHORT]], i64 8, [[TYPE_INT]], i64 12}
 // OLD-PATH: [[TAG_C_b_a_f32]] = !{[[TYPE_C:!.*]], [[TYPE_INT]], i64 12}
 // OLD-PATH: [[TYPE_C]] = !{!"_ZTS7StructC", [[TYPE_SHORT]], i64 0, [[TYPE_B]], i64 4, [[TYPE_INT]], i64 28}
 // OLD-PATH: [[TAG_D_b_a_f32]] = !{[[TYPE_D:!.*]], [[TYPE_INT]], i64 12}
 // OLD-PATH: [[TYPE_D]] = !{!"_ZTS7StructD", [[TYPE_SHORT]], i64 0, [[TYPE_B]], i64 4, [[TYPE_INT]], i64 28, [[TYPE_CHAR]], i64 32}
+// OLD-PATH: [[TAG_M1_f16_2]] = !{[[TYPE_M1:!.*]], [[TYPE_SHORT]], i64 12}
+// OLD-PATH: [[TYPE_M1]] = !{!"_ZTS8StructM1", [[TYPE_S]], i64 0, [[TYPE_T:!.*]], i64 8, [[TYPE_SHORT]], i64 12}
+// OLD_PATH: [[TYPE_T]] = !{!"_ZTS7StructT", [[TYPE_INT]], i64 0}
+// OLD-PATH: [[TAG_M2_f16_2]] = !{[[TYPE_M2:!.*]], [[TYPE_SHORT]], i64 20}
+// OLD-PATH: [[TYPE_M2]] = !{!"_ZTS8StructM2", [[TYPE_DYN:!.*]], i64 0, [[TYPE_S]], i64 12, [[TYPE_SHORT]], i64 20}
+// OLD_PATH: [[TYPE_DYN]] = !{!"_ZTS9StructDyn", [[TYPE_INT]], i64 8}
 
 // NEW-PATH: [[TYPE_CHAR:!.*]] = !{!{{.*}}, i64 1, !"omnipotent char"}
 // NEW-PATH: [[

[PATCH] D88363: [CodeGen] Improve likelihood attribute branch weights

[Bruno De Fraine via Phabricator via cfe-commits]

bdf accepted this revision.
bdf added a comment.
This revision is now accepted and ready to land.

Looks good. Good idea to add tests to verify that we match `__builtin_except()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88363

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

[PATCH] D88363: [CodeGen] Improve likelihood attribute branch weights

[Bruno De Fraine via Phabricator via cfe-commits]

bdf added a comment.

In D88363#2319157 , @jmorse wrote:

> In D88363#2317241 , @Mordante wrote:
>
>> Can you explain the kind of issues you're having?
>
> At the shallowest level, our -O1 produces different IR and fails the test, 
> which is more or less our problem; however my understanding is that tests in 
> the LLVM project / subprojects should aim to test as little amount of code as 
> possible. Relying on all of -O1 makes it a brittle test -- changes to any 
> optimisation pass enabled in -O1 could cause this test to fail spuriously.
>
> Instead, I believe the test should be in two parts:
>
> - One checking clang produces the correct /unoptimised/ IR output
> - One or more checking that the consuming IR passes do-the-right-thing

As I see, the intent of the test is not so much to verify a certain expected 
output, but more to verify that two styles of likelihood hints in C code 
produce the same code structure and branch weights. Theses styles are 
likely/unlikely-annotations, and use of __builtin_expect in the if condition. 
But the processing of these two is quite different:

- for likely/unlikely annotations, branch weights are added immediately in the 
initial CodeGen
- __builtin_expect is first translated straightforward to an expect intrinsic, 
then processed by a later lower-expect pass

To make the test less brittle, would it be possible to explicitly select only 
the optimization passes that are needed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88363

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

[PATCH] D104777: PR50767: clear non-distinct debuginfo for function with nodebug definition after undecorated declaration

[Bruno De Fraine via Phabricator via cfe-commits]

brunodefraine created this revision.
brunodefraine added reviewers: ychen, dblaikie.
brunodefraine added a project: clang.
brunodefraine requested review of this revision.
Herald added a subscriber: cfe-commits.

Fix suggested by Yuanfang Chen:

Non-distinct debuginfo is attached to the function due to the undecorated 
declaration. Later, when seeing the function definition and `nodebug` 
attribute, the non-distinct debuginfo should be cleared.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D104777

Files:
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/test/CodeGen/attr-nodebug2.cpp


Index: clang/test/CodeGen/attr-nodebug2.cpp
===
--- /dev/null
+++ clang/test/CodeGen/attr-nodebug2.cpp
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -debug-info-kind=limited -debugger-tuning=gdb 
-dwarf-version=4 -O -disable-llvm-passes -emit-llvm -o - %s | FileCheck %s
+
+void t1();
+
+void use() { t1(); }
+
+__attribute__((nodebug))
+void t1()
+{
+  int a = 10;
+  a++;
+}
+
+// CHECK-LABEL: define{{.*}} void @_Z3usev()
+// CHECK-SAME:  !dbg
+// CHECK-SAME:  {
+// CHECK:   !dbg
+// CHECK:   }
+
+// PR50767 Function __attribute__((nodebug)) inconsistency causes crash
+// illegal (non-distinct) !dbg metadata was being added to _Z2t1v definition
+
+// CHECK-LABEL: define{{.*}} void @_Z2t1v()
+// CHECK-NOT:   !dbg
+// CHECK-SAME:  {
+// CHECK-NOT:   !dbg
+// CHECK:   }
Index: clang/lib/CodeGen/CodeGenFunction.cpp
===
--- clang/lib/CodeGen/CodeGenFunction.cpp
+++ clang/lib/CodeGen/CodeGenFunction.cpp
@@ -1300,8 +1300,14 @@
   QualType ResTy = BuildFunctionArgList(GD, Args);
 
   // Check if we should generate debug info for this function.
-  if (FD->hasAttr())
-DebugInfo = nullptr; // disable debug info indefinitely for this function
+  if (FD->hasAttr()) {
+// Clear non-distinct debug info that was possibly attached to
+// the function due to a declaration with the nodebug attribute
+if (Fn)
+  Fn->setSubprogram(nullptr);
+// Disable debug info indefinitely for this function
+DebugInfo = nullptr;
+  }
 
   // The function might not have a body if we're generating thunks for a
   // function declaration.


Index: clang/test/CodeGen/attr-nodebug2.cpp
===
--- /dev/null
+++ clang/test/CodeGen/attr-nodebug2.cpp
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -debug-info-kind=limited -debugger-tuning=gdb -dwarf-version=4 -O -disable-llvm-passes -emit-llvm -o - %s | FileCheck %s
+
+void t1();
+
+void use() { t1(); }
+
+__attribute__((nodebug))
+void t1()
+{
+  int a = 10;
+  a++;
+}
+
+// CHECK-LABEL: define{{.*}} void @_Z3usev()
+// CHECK-SAME:  !dbg
+// CHECK-SAME:  {
+// CHECK:   !dbg
+// CHECK:   }
+
+// PR50767 Function __attribute__((nodebug)) inconsistency causes crash
+// illegal (non-distinct) !dbg metadata was being added to _Z2t1v definition
+
+// CHECK-LABEL: define{{.*}} void @_Z2t1v()
+// CHECK-NOT:   !dbg
+// CHECK-SAME:  {
+// CHECK-NOT:   !dbg
+// CHECK:   }
Index: clang/lib/CodeGen/CodeGenFunction.cpp
===
--- clang/lib/CodeGen/CodeGenFunction.cpp
+++ clang/lib/CodeGen/CodeGenFunction.cpp
@@ -1300,8 +1300,14 @@
   QualType ResTy = BuildFunctionArgList(GD, Args);
 
   // Check if we should generate debug info for this function.
-  if (FD->hasAttr())
-DebugInfo = nullptr; // disable debug info indefinitely for this function
+  if (FD->hasAttr()) {
+// Clear non-distinct debug info that was possibly attached to
+// the function due to a declaration with the nodebug attribute
+if (Fn)
+  Fn->setSubprogram(nullptr);
+// Disable debug info indefinitely for this function
+DebugInfo = nullptr;
+  }
 
   // The function might not have a body if we're generating thunks for a
   // function declaration.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104777: PR50767: clear non-distinct debuginfo for function with nodebug definition after undecorated declaration

[Bruno De Fraine via Phabricator via cfe-commits]

brunodefraine updated this revision to Diff 353949.
brunodefraine added a comment.

Fix issues from Windows/clang-format buildbot. Fix mistake in code comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104777

Files:
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/test/CodeGen/attr-nodebug2.cpp


Index: clang/test/CodeGen/attr-nodebug2.cpp
===
--- /dev/null
+++ clang/test/CodeGen/attr-nodebug2.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -debug-info-kind=limited 
-debugger-tuning=gdb -dwarf-version=4 -O -disable-llvm-passes -emit-llvm -o - 
%s | FileCheck %s
+
+void t1();
+
+void use() { t1(); }
+
+__attribute__((nodebug)) void t1() {
+  int a = 10;
+  a++;
+}
+
+// CHECK-LABEL: define{{.*}} void @_Z3usev()
+// CHECK-SAME:  !dbg
+// CHECK-SAME:  {
+// CHECK:   !dbg
+// CHECK:   }
+
+// PR50767 Function __attribute__((nodebug)) inconsistency causes crash
+// illegal (non-distinct) !dbg metadata was being added to _Z2t1v definition
+
+// CHECK-LABEL: define{{.*}} void @_Z2t1v()
+// CHECK-NOT:   !dbg
+// CHECK-SAME:  {
+// CHECK-NOT:   !dbg
+// CHECK:   }
Index: clang/lib/CodeGen/CodeGenFunction.cpp
===
--- clang/lib/CodeGen/CodeGenFunction.cpp
+++ clang/lib/CodeGen/CodeGenFunction.cpp
@@ -1300,8 +1300,14 @@
   QualType ResTy = BuildFunctionArgList(GD, Args);
 
   // Check if we should generate debug info for this function.
-  if (FD->hasAttr())
-DebugInfo = nullptr; // disable debug info indefinitely for this function
+  if (FD->hasAttr()) {
+// Clear non-distinct debug info that was possibly attached to the function
+// due to an earlier declaration without the nodebug attribute
+if (Fn)
+  Fn->setSubprogram(nullptr);
+// Disable debug info indefinitely for this function
+DebugInfo = nullptr;
+  }
 
   // The function might not have a body if we're generating thunks for a
   // function declaration.


Index: clang/test/CodeGen/attr-nodebug2.cpp
===
--- /dev/null
+++ clang/test/CodeGen/attr-nodebug2.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -debug-info-kind=limited -debugger-tuning=gdb -dwarf-version=4 -O -disable-llvm-passes -emit-llvm -o - %s | FileCheck %s
+
+void t1();
+
+void use() { t1(); }
+
+__attribute__((nodebug)) void t1() {
+  int a = 10;
+  a++;
+}
+
+// CHECK-LABEL: define{{.*}} void @_Z3usev()
+// CHECK-SAME:  !dbg
+// CHECK-SAME:  {
+// CHECK:   !dbg
+// CHECK:   }
+
+// PR50767 Function __attribute__((nodebug)) inconsistency causes crash
+// illegal (non-distinct) !dbg metadata was being added to _Z2t1v definition
+
+// CHECK-LABEL: define{{.*}} void @_Z2t1v()
+// CHECK-NOT:   !dbg
+// CHECK-SAME:  {
+// CHECK-NOT:   !dbg
+// CHECK:   }
Index: clang/lib/CodeGen/CodeGenFunction.cpp
===
--- clang/lib/CodeGen/CodeGenFunction.cpp
+++ clang/lib/CodeGen/CodeGenFunction.cpp
@@ -1300,8 +1300,14 @@
   QualType ResTy = BuildFunctionArgList(GD, Args);
 
   // Check if we should generate debug info for this function.
-  if (FD->hasAttr())
-DebugInfo = nullptr; // disable debug info indefinitely for this function
+  if (FD->hasAttr()) {
+// Clear non-distinct debug info that was possibly attached to the function
+// due to an earlier declaration without the nodebug attribute
+if (Fn)
+  Fn->setSubprogram(nullptr);
+// Disable debug info indefinitely for this function
+DebugInfo = nullptr;
+  }
 
   // The function might not have a body if we're generating thunks for a
   // function declaration.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104777: PR50767: clear non-distinct debuginfo for function with nodebug definition after undecorated declaration

[Bruno De Fraine via Phabricator via cfe-commits]

brunodefraine added a subscriber: probinson.
brunodefraine added a comment.

In D104777#2836669 , @dblaikie wrote:

> Yeah, all that sounds reasonable to me - @brunodefraine could you look into 
> supporting nodebug in a similar way as @aaron.ballman has described here?

Since the debuginfo for `use()` is slightly affected by the `nodebug` version 
of `t1()` that follows it, I can see how this back propagation is perhaps 
dangerous. Checking that `nodebug` is the same on all declarations of a 
function is a way to prevent this.

But when discussing the PR, @probinson wrote "I'm inclined to think we want 
this to work" and I can see what he means from the use case where I observed 
the bug. If you don't want debuginfo for the implementation of `t1()`, it 
should be fine to annotate just the function definition in an implementation 
file, not the declaration in a header, since the debuginfo of the 
implementation is not of the caller's concern. But `nodebug` as it exists 
**does** affect the debuginfo of callers as well, so I cannot really express 
that I don't want debuginfo for the implementation of a function and leave its 
callers unaffected?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104777

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

[PATCH] D104777: PR50767: clear non-distinct debuginfo for function with nodebug definition after undecorated declaration

[Bruno De Fraine via Phabricator via cfe-commits]

brunodefraine added a comment.

Following suggestion by @dblaikie and @aaron.ballman I'm preparing a new diff 
where a function redeclaration that adds `nodebug` is flagged as an error in 
`Sema::mergeDeclAttributes`.

This triggers test failures because of violations in builtin header files, for 
example:

  In file included from clang/test/CodeGen/ms-intrinsics.c:18:
  build/lib/clang/13.0.0/include/intrin.h:451:24: error: function declared with 
'nodebug' attribute was previously declared without the 'nodebug' attribute
  static __inline__ void __DEFAULT_FN_ATTRS __movsb(unsigned char *__dst,
 ^
  build/lib/clang/13.0.0/include/intrin.h:37:62: note: expanded from macro 
'__DEFAULT_FN_ATTRS'
  #define __DEFAULT_FN_ATTRS __attribute__((__always_inline__, __nodebug__))
   ^
  build/lib/clang/13.0.0/include/intrin.h:80:6: note: previous declaration is 
here
  void __movsb(unsigned char *, unsigned char const *, size_t);
   ^

From what I understand from `clang/lib/Headers/intrin.h` the functions are 
declared in general at the beginning of the headers, then implemented for 
specific architectures as inline function containing assembly. I do not know if 
it is appropriate to mark the general declarations as `nodebug`...

That I already encounter this in builtin headers is probably an indication that 
the new error will break some code in the wild. A pragmatic solution would be 
to only raise the error in `Sema::mergeDeclAttributes` if `Old->isUsed()`, i.e. 
an earlier declaration without `nodebug` can be forgiven if not yet used. I 
think that would still make it impossible to trigger the crash?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104777

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

[PATCH] D104777: PR50767: clear non-distinct debuginfo for function with nodebug definition after undecorated declaration

[Bruno De Fraine via Phabricator via cfe-commits]

brunodefraine updated this revision to Diff 354922.
brunodefraine added a comment.

Slightly improved test case (both C and C++).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104777

Files:
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/test/CodeGen/attr-nodebug2.c


Index: clang/test/CodeGen/attr-nodebug2.c
===
--- /dev/null
+++ clang/test/CodeGen/attr-nodebug2.c
@@ -0,0 +1,34 @@
+// RUN: %clang_cc1 -x c -debug-info-kind=limited -debugger-tuning=gdb 
-dwarf-version=4 -O -disable-llvm-passes -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -x c++ -debug-info-kind=limited -debugger-tuning=gdb 
-dwarf-version=4 -O -disable-llvm-passes -emit-llvm -o - %s | FileCheck %s
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+void t1();
+
+void use() { t1(); }
+
+__attribute__((nodebug)) void t1() {
+  int a = 10;
+  a++;
+}
+
+#ifdef __cplusplus
+}
+#endif
+
+// CHECK-LABEL: define{{.*}} void @use()
+// CHECK-SAME:  !dbg
+// CHECK-SAME:  {
+// CHECK:   !dbg
+// CHECK:   }
+
+// PR50767 Function __attribute__((nodebug)) inconsistency causes crash
+// illegal (non-distinct) !dbg metadata was being added to _Z2t1v definition
+
+// CHECK-LABEL: define{{.*}} void @t1()
+// CHECK-NOT:   !dbg
+// CHECK-SAME:  {
+// CHECK-NOT:   !dbg
+// CHECK:   }
Index: clang/lib/CodeGen/CodeGenFunction.cpp
===
--- clang/lib/CodeGen/CodeGenFunction.cpp
+++ clang/lib/CodeGen/CodeGenFunction.cpp
@@ -1304,8 +1304,14 @@
   QualType ResTy = BuildFunctionArgList(GD, Args);
 
   // Check if we should generate debug info for this function.
-  if (FD->hasAttr())
-DebugInfo = nullptr; // disable debug info indefinitely for this function
+  if (FD->hasAttr()) {
+// Clear non-distinct debug info that was possibly attached to the function
+// due to an earlier declaration without the nodebug attribute
+if (Fn)
+  Fn->setSubprogram(nullptr);
+// Disable debug info indefinitely for this function
+DebugInfo = nullptr;
+  }
 
   // The function might not have a body if we're generating thunks for a
   // function declaration.


Index: clang/test/CodeGen/attr-nodebug2.c
===
--- /dev/null
+++ clang/test/CodeGen/attr-nodebug2.c
@@ -0,0 +1,34 @@
+// RUN: %clang_cc1 -x c -debug-info-kind=limited -debugger-tuning=gdb -dwarf-version=4 -O -disable-llvm-passes -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -x c++ -debug-info-kind=limited -debugger-tuning=gdb -dwarf-version=4 -O -disable-llvm-passes -emit-llvm -o - %s | FileCheck %s
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+void t1();
+
+void use() { t1(); }
+
+__attribute__((nodebug)) void t1() {
+  int a = 10;
+  a++;
+}
+
+#ifdef __cplusplus
+}
+#endif
+
+// CHECK-LABEL: define{{.*}} void @use()
+// CHECK-SAME:  !dbg
+// CHECK-SAME:  {
+// CHECK:   !dbg
+// CHECK:   }
+
+// PR50767 Function __attribute__((nodebug)) inconsistency causes crash
+// illegal (non-distinct) !dbg metadata was being added to _Z2t1v definition
+
+// CHECK-LABEL: define{{.*}} void @t1()
+// CHECK-NOT:   !dbg
+// CHECK-SAME:  {
+// CHECK-NOT:   !dbg
+// CHECK:   }
Index: clang/lib/CodeGen/CodeGenFunction.cpp
===
--- clang/lib/CodeGen/CodeGenFunction.cpp
+++ clang/lib/CodeGen/CodeGenFunction.cpp
@@ -1304,8 +1304,14 @@
   QualType ResTy = BuildFunctionArgList(GD, Args);
 
   // Check if we should generate debug info for this function.
-  if (FD->hasAttr())
-DebugInfo = nullptr; // disable debug info indefinitely for this function
+  if (FD->hasAttr()) {
+// Clear non-distinct debug info that was possibly attached to the function
+// due to an earlier declaration without the nodebug attribute
+if (Fn)
+  Fn->setSubprogram(nullptr);
+// Disable debug info indefinitely for this function
+DebugInfo = nullptr;
+  }
 
   // The function might not have a body if we're generating thunks for a
   // function declaration.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100136: Allow applying attributes to subset of allowed subjects.

[Bruno De Fraine via Phabricator via cfe-commits]

brunodf added a comment.

This change is important make better use of `#pragma clang attribute` (which is 
cool!), but for some uses the check still seems to restrictive.

Concretely, for an attribute where `Subjects` includes `FunctionLike`, I can 
only match it using the rule `hasType(functionType)` but not with a `function` 
rule. E.g. if I want to apply it to C++ methods, the rule `function(is_member)` 
is rejected (as `attribute ... can't be applied to 'function(is_member)'`).

If I change the subject list of my attribute to include `Function` in addition 
to `FunctionLike`, it does work, but I don't know if there are other effects.

I'm not exactly sure what is the purpose of the check, since there is already a 
warning when your pragma does not match anything? If an attribute is updated to 
allow more subjects, how does that affect matching of existing rules?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100136

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

[PATCH] D111639: [Sema] check PseudoObject when rebuilding CXXOperatorCallExpr in template instantiation

[Bruno De Fraine via Phabricator via cfe-commits]

brunodf added a comment.



In D111639#3081504 , @brunodf wrote:

> With regard to OO_Subscript, OO_Arrow and OO_Amp, I have tried a number of 
> things, but I have not succeeded in triggering that an CXXOperatorCallExpr is 
> created for these operators (they end up as an ArraySubscriptExpr, 
> CXXDependentScopeMemberExpr and UnaryOperator respectively). At the moment, I 
> don't know how to test the code paths for these operators in 
> RebuildCXXOperatorCallExpr.

After my submission, I still looked further into cases with these operators, 
but a CXXOperatorCallExpr is only used for these operators in case an 
overloaded operator has been determined (so not in case of a dependent type). 
Also, these operators are not allowed as non-members functions. While a 
property member could be of a type that has such an overloaded member operator, 
this would be dubious since it would work on a reference to the temporary 
returned by the getter of the property member.

So while it would be possible to add tests for CXXOperatorCallExpr involving 
invocation of these operators on property members, I think this would be 
artificial and not really contribute to the patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111639

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

[PATCH] D126956: [tbaa] Handle base classes in struct tbaa

[Bruno De Fraine via Phabricator via cfe-commits]

brunodf added a comment.

Anyone?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126956

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

[PATCH] D126956: [tbaa] Handle base classes in struct tbaa

[Bruno De Fraine via Phabricator via cfe-commits]

brunodf created this revision.
brunodf added reviewers: jeroen.dobbelaere, eli.friedman.
Herald added a subscriber: kosarev.
Herald added a project: All.
brunodf requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This is a fix for the miscompilation reported in 
https://github.com/llvm/llvm-project/issues/55384

Not adding a new test case since existing test cases already cover base classes 
(including new-struct-path tbaa).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D126956

Files:
  clang/lib/CodeGen/CodeGenTBAA.cpp
  clang/test/CodeGen/tbaa-class.cpp
  clang/unittests/CodeGen/TBAAMetadataTest.cpp


Index: clang/unittests/CodeGen/TBAAMetadataTest.cpp
===
--- clang/unittests/CodeGen/TBAAMetadataTest.cpp
+++ clang/unittests/CodeGen/TBAAMetadataTest.cpp
@@ -968,13 +968,10 @@
   MConstInt(0)),
 MConstInt(0));
 
-  auto ClassDerived = MMTuple(
-MMString("_ZTS7Derived"),
-MMTuple(
-  MMString("short"),
-  OmnipotentCharCXX,
-  MConstInt(0)),
-MConstInt(4));
+  auto ClassDerived =
+  MMTuple(MMString("_ZTS7Derived"), ClassBase, MConstInt(0),
+  MMTuple(MMString("short"), OmnipotentCharCXX, MConstInt(0)),
+  MConstInt(4));
 
   const Instruction *I = match(BB,
   MInstruction(Instruction::Store,
@@ -1047,13 +1044,10 @@
   MConstInt(0)),
 MConstInt(Compiler.PtrSize));
 
-  auto ClassDerived = MMTuple(
-MMString("_ZTS7Derived"),
-MMTuple(
-  MMString("short"),
-  OmnipotentCharCXX,
-  MConstInt(0)),
-MConstInt(Compiler.PtrSize + 4));
+  auto ClassDerived =
+  MMTuple(MMString("_ZTS7Derived"), ClassBase, MConstInt(0),
+  MMTuple(MMString("short"), OmnipotentCharCXX, MConstInt(0)),
+  MConstInt(Compiler.PtrSize + 4));
 
   const Instruction *I = match(BB,
   MInstruction(Instruction::Store,
Index: clang/test/CodeGen/tbaa-class.cpp
===
--- clang/test/CodeGen/tbaa-class.cpp
+++ clang/test/CodeGen/tbaa-class.cpp
@@ -222,7 +222,7 @@
 // OLD-PATH: [[TYPE_S]] = !{!"_ZTS7StructS", [[TYPE_SHORT]], i64 0, 
[[TYPE_INT]], i64 4}
 // OLD-PATH: [[TAG_S_f16]] = !{[[TYPE_S]], [[TYPE_SHORT]], i64 0}
 // OLD-PATH: [[TAG_S2_f32_2]] = !{[[TYPE_S2:!.*]], [[TYPE_INT]], i64 12}
-// OLD-PATH: [[TYPE_S2]] = !{!"_ZTS8StructS2", [[TYPE_SHORT]], i64 8, 
[[TYPE_INT]], i64 12}
+// OLD-PATH: [[TYPE_S2]] = !{!"_ZTS8StructS2", [[TYPE_S]], i64 0, 
[[TYPE_SHORT]], i64 8, [[TYPE_INT]], i64 12}
 // OLD-PATH: [[TAG_C_b_a_f32]] = !{[[TYPE_C:!.*]], [[TYPE_INT]], i64 12}
 // OLD-PATH: [[TYPE_C]] = !{!"_ZTS7StructC", [[TYPE_SHORT]], i64 0, 
[[TYPE_B]], i64 4, [[TYPE_INT]], i64 28}
 // OLD-PATH: [[TAG_D_b_a_f32]] = !{[[TYPE_D:!.*]], [[TYPE_INT]], i64 12}
@@ -244,7 +244,7 @@
 // NEW-PATH: [[TYPE_S]] = !{[[TYPE_CHAR]], i64 8, !"_ZTS7StructS", 
[[TYPE_SHORT]], i64 0, i64 2, [[TYPE_INT]], i64 4, i64 4}
 // NEW-PATH: [[TAG_S_f16]] = !{[[TYPE_S]], [[TYPE_SHORT]], i64 0, i64 2}
 // NEW-PATH: [[TAG_S2_f32_2]] = !{[[TYPE_S2:!.*]], [[TYPE_INT]], i64 12, i64 4}
-// NEW-PATH: [[TYPE_S2]] = !{[[TYPE_CHAR]], i64 16, !"_ZTS8StructS2", 
[[TYPE_SHORT]], i64 8, i64 2, [[TYPE_INT]], i64 12, i64 4}
+// NEW-PATH: [[TYPE_S2]] = !{[[TYPE_CHAR]], i64 16, !"_ZTS8StructS2", 
[[TYPE_S]], i64 0, i64 8, [[TYPE_SHORT]], i64 8, i64 2, [[TYPE_INT]], i64 12, 
i64 4}
 // NEW-PATH: [[TAG_C_b_a_f32]] = !{[[TYPE_C:!.*]], [[TYPE_INT]], i64 12, i64 4}
 // NEW-PATH: [[TYPE_C]] = !{[[TYPE_CHAR]], i64 32, !"_ZTS7StructC", 
[[TYPE_SHORT]], i64 0, i64 2, [[TYPE_B]], i64 4, i64 24, [[TYPE_INT]], i64 28, 
i64 4}
 // NEW-PATH: [[TAG_D_b_a_f32]] = !{[[TYPE_D:!.*]], [[TYPE_INT]], i64 12, i64 4}
Index: clang/lib/CodeGen/CodeGenTBAA.cpp
===
--- clang/lib/CodeGen/CodeGenTBAA.cpp
+++ clang/lib/CodeGen/CodeGenTBAA.cpp
@@ -336,6 +336,30 @@
 const RecordDecl *RD = TTy->getDecl()->getDefinition();
 const ASTRecordLayout &Layout = Context.getASTRecordLayout(RD);
 SmallVector Fields;
+if (const CXXRecordDecl *CXXRD = dyn_cast(RD)) {
+  // Handle C++ base classes. Non-virtual bases can treated a a kind of
+  // field. Virtual bases are more complex and omitted, but avoid an
+  // incomplete view for NewStructPathTBAA.
+  if (CodeGenOpts.NewStructPathTBAA && CXXRD->getNumVBases() != 0)
+return BaseTypeMetadataCache[Ty] = nullptr;
+  for (const CXXBaseSpecifier &B : CXXRD->bases()) {
+if (B.isVirtual())
+  continue;
+QualType BaseQTy = B.getType();
+const CXXRecordDecl *BaseRD = BaseQTy->getAsCXXRecordDecl();
+if (BaseRD->isEmpty())
+  continue;
+llvm::MDNode *TypeNode = isValidBaseType(BaseQTy)
+ ? getBaseTypeInfo(BaseQTy)
+ : getTypeInfo(BaseQTy);
+   

[PATCH] D111639: [Sema] check PseudoObject when rebuilding CXXOperatorCallExpr in template instantiation

[Bruno De Fraine via Phabricator via cfe-commits]

brunodf added inline comments.



Comment at: clang/lib/Sema/TreeTransform.h:14583
 
   if (Second && Second->getObjectKind() == OK_ObjCProperty) {
 ExprResult Result = SemaRef.CheckPlaceholderExpr(Second);

rjmccall wrote:
> rnk wrote:
> > This is also pseudo object handling code
> Hmm.  Am I wrong to be concerned about folding overload placeholders too 
> early in these clauses?  Surely overloads can be resolved by the operator 
> call in some cases.
> 
> I agree with Reid that it would be really nice if we could make this share 
> the normal paths for C++ operator resolution instead of duplicating so much 
> of them.
> Hmm.  Am I wrong to be concerned about folding overload placeholders too 
> early in these clauses?  Surely overloads can be resolved by the operator 
> call in some cases.

This would have to be reviewed against the paths in `Sema::BuildBinOp`? There 
are some cases for a RHS placeholder that does not end in calling 
`CheckPlaceholderExpr`:


```
 // Handle pseudo-objects in the RHS.
  if (const BuiltinType *pty = RHSExpr->getType()->getAsPlaceholderType()) {
// An overload in the RHS can potentially be resolved by the type
// being assigned to.
if (Opc == BO_Assign && pty->getKind() == BuiltinType::Overload) {
  if (getLangOpts().CPlusPlus &&
  (LHSExpr->isTypeDependent() || RHSExpr->isTypeDependent() ||
   LHSExpr->getType()->isOverloadableType()))
return BuildOverloadedBinOp(*this, S, OpLoc, Opc, LHSExpr, RHSExpr);

  return CreateBuiltinBinOp(OpLoc, Opc, LHSExpr, RHSExpr);
}

// Don't resolve overloads if the other type is overloadable.
if (getLangOpts().CPlusPlus && pty->getKind() == BuiltinType::Overload &&
LHSExpr->getType()->isOverloadableType())
  return BuildOverloadedBinOp(*this, S, OpLoc, Opc, LHSExpr, RHSExpr);

ExprResult resolvedRHS = CheckPlaceholderExpr(RHSExpr);
if (!resolvedRHS.isUsable()) return ExprError();
RHSExpr = resolvedRHS.get();
  }
```

> I agree with Reid that it would be really nice if we could make this share 
> the normal paths for C++ operator resolution instead of duplicating so much 
> of them.

I think extracting the common placeholder logic from Build(Unary|Bin)Op and 
this function is beyond the scope of this patch (and probably outside my 
capabilities, I'm afraid). Still I hope to get things in a better shape with 
this patch than it was before (see PR51855). A follow up that would attempt to 
remove the duplication would indeed be nice.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111639

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

[PATCH] D111639: [Sema] check PseudoObject when rebuilding CXXOperatorCallExpr in template instantiation

[Bruno De Fraine via Phabricator via cfe-commits]

brunodf updated this revision to Diff 392429.
brunodf added a comment.

New version of patch to address problems detected by buildbots.

(Will describe in more detail in follow-up comment.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111639

Files:
  clang/lib/Sema/TreeTransform.h
  clang/test/SemaCXX/D111639.cpp
  clang/test/SemaCXX/PR51855.cpp
  clang/test/SemaObjCXX/instantiate-property-access.mm

Index: clang/test/SemaObjCXX/instantiate-property-access.mm
===
--- clang/test/SemaObjCXX/instantiate-property-access.mm
+++ clang/test/SemaObjCXX/instantiate-property-access.mm
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -DDEPENDENT -verify %s
 // expected-no-diagnostics
 
 class C {};
@@ -9,6 +10,10 @@
 
 C operator += (C c1, C c2);
 
+C operator++(C c1);
+
+bool operator!(C c1);
+
 enum TextureType { TextureType3D  };
 
 @interface Texture
@@ -16,9 +21,13 @@
 @property  C c;
 @end
 
-template  class Framebuffer {
+template  class Framebuffer {
 public:
-  Texture **color_attachment;  
+#ifdef DEPENDENT
+  T **color_attachment;
+#else
+  Texture **color_attachment;
+#endif
   Framebuffer();
 };
 
@@ -28,8 +37,15 @@
   (void)(color_attachment[0].c == color_attachment[0].c);
   (void)(color_attachment[0].c == 1);
   (void)(1 == color_attachment[0].c);
+  (void)(!color_attachment[0].textureType);
+  ++color_attachment[0].textureType;
+  (void)(!color_attachment[0].c);
 }
 
 void foo() {
+#ifdef DEPENDENT
+  Framebuffer();
+#else
   Framebuffer();
+#endif
 }
Index: clang/test/SemaCXX/PR51855.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/PR51855.cpp
@@ -0,0 +1,71 @@
+// RUN: %clang_cc1 -S -triple %itanium_abi_triple -fms-extensions -emit-llvm %s -o - | FileCheck %s
+
+struct F {};
+
+F operator*=(F &lhs, int rhs);
+
+F operator++(F &lhs);
+
+struct S {
+  short _m;
+  S(short _m) : _m(_m) {}
+
+  void putM(short rhs) { _m = rhs; }
+  short getM() { return _m; }
+
+  __declspec(property(get = getM, put = putM)) short theData;
+};
+
+int test1a(int i) {
+  S tmp(i);
+  tmp.theData *= 2;
+  return tmp.theData;
+}
+
+// CHECK-LABEL: define {{.*}} @_Z6test1ai(
+// CHECK: call {{.*}} @_ZN1SC1Es(
+// CHECK: call {{.*}} @_ZN1S4getMEv(
+// CHECK: call {{.*}} @_ZN1S4putMEs(
+// CHECK: call {{.*}} @_ZN1S4getMEv(
+
+template 
+int test1b(int i) {
+  T tmp(i);
+  tmp.theData *= 2;
+  return tmp.theData;
+}
+
+template int test1b(int);
+
+// CHECK-LABEL: define {{.*}} @_Z6test1bI1SEii(
+// CHECK: call {{.*}} @_ZN1SC1Es(
+// CHECK: call {{.*}} @_ZN1S4getMEv(
+// CHECK: call {{.*}} @_ZN1S4putMEs(
+// CHECK: call {{.*}} @_ZN1S4getMEv(
+
+int test2a(int i) {
+  S tmp(i);
+  ++tmp.theData;
+  return tmp.theData;
+}
+
+// CHECK-LABEL: define {{.*}} i32 @_Z6test2ai(
+// CHECK: call {{.*}} @_ZN1SC1Es(
+// CHECK: call {{.*}} @_ZN1S4getMEv(
+// CHECK: call {{.*}} @_ZN1S4putMEs(
+// CHECK: call {{.*}} @_ZN1S4getMEv(
+
+template 
+int test2b(int i) {
+  T tmp(i);
+  ++tmp.theData;
+  return tmp.theData;
+}
+
+template int test2b(int);
+
+// CHECK-LABEL: define {{.*}} i32 @_Z6test2bI1SEii(
+// CHECK: call {{.*}} @_ZN1SC1Es(
+// CHECK: call {{.*}} @_ZN1S4getMEv(
+// CHECK: call {{.*}} @_ZN1S4putMEs(
+// CHECK: call {{.*}} @_ZN1S4getMEv(
Index: clang/test/SemaCXX/D111639.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/D111639.cpp
@@ -0,0 +1,51 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// expected-no-diagnostics
+
+// Test case for an issue that appeared while developing D111639:
+//
+// An earlier version of the diff unexpectedly caused compilation of
+// llvm/Support/AllocatorBase.h and llvm/Support/Allocator.h to fail in
+// multistage buildbots.  This is a reduced version of the construct that
+// caused the problem.
+
+using size_t = __SIZE_TYPE__;
+
+struct X {
+  int i;
+};
+
+X operator&(X, X); // binary operator& needed to fill (non-member) overload set of unary operator&
+
+template 
+class Base {
+public:
+  void *Alloc(size_t Size) {
+static_assert(
+static_cast(&Base::Alloc) != static_cast(&Derived::Alloc),
+"must override");
+return static_cast(this)->Alloc(Size);
+  }
+  template 
+  T *Alloc() {
+return static_cast(Alloc(sizeof(T)));
+  }
+};
+
+class Sub : public Base {
+public:
+  void *Alloc(int);
+  void *Alloc(size_t Size) {
+return Alloc((int)Size);
+  }
+  using Base::Alloc;
+};
+
+Sub s;
+
+void *test() {
+  return s.Alloc((size_t)123);
+}
+
+char *test2() {
+  return s.Alloc();
+}
Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -14630,18 +14630,37 @@
   Expr *Callee = OrigCallee->IgnoreParenCasts();
   bool isPostIncDec = Second

[PATCH] D111639: [Sema] check PseudoObject when rebuilding CXXOperatorCallExpr in template instantiation

[Bruno De Fraine via Phabricator via cfe-commits]

brunodf reopened this revision.
brunodf added a comment.
This revision is now accepted and ready to land.

When the previous diff was committed, two problems were discovered in the 
buildbots:

1. One pattern in the `PR51855.cpp` test failed on certain targets due to a 
difference in calling convention: 
https://lab.llvm.org/buildbot/#/builders/171/builds/6632
2. In multistage buildbots, an overload resolution failure was reported while 
compiling the LLVM codebase in stage 2: 
https://lab.llvm.org/buildbot/#/builders/187/builds/3136

The pattern in the `PR51855.cpp` test is straightforward to generalize.

The stage 2 issue revealed a problem that is now reduced in the `D111639.cpp` 
test. Here, a unary address-of `&` operator is used on a 
`DependentScopeDeclRef` (which has dependent type), and the presence of a 
binary `operator&` overload triggers the use of a `CXXOperatorCallExpr`. When 
the `CXXOperatorCallExpr` is rebuilt for template instantiation, the argument 
is an overload set. It has a placeholder type (`BuiltinType::Overload`), but 
does not respond well to `CheckPlaceHolderExpr` invocation: this triggers the 
"reference to overloaded function could not be resolved" error.

In `Sema::BuildUnaryOp` there is an extra test clause for `UO_AddrOf` operators 
in the logic for placeholder types, before calling `CheckPlaceHolderExpr`. I've 
adopted the same clause here.

Before this diff, there was no initial check for placeholder types (only for 
`OK_ObjCProperty`) and this would be handled by lines 14661-14669 of 
`TreeTransform.h` (left-hand side numbering), also resulting in a call to 
`CreateBuiltinUnaryOp`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111639

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

[PATCH] D111639: [Sema] check PseudoObject when rebuilding CXXOperatorCallExpr in template instantiation

[Bruno De Fraine via Phabricator via cfe-commits]

brunodf requested review of this revision.
brunodf added a comment.

A stage2 build (outside of the buildloops) is now working correctly. Pinging 
for new review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111639

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

[PATCH] D111639: [PR51855] check PseudoObject when rebuilding CXXOperatorCallExpr in template instantiation

[Bruno De Fraine via Phabricator via cfe-commits]

brunodf created this revision.
brunodf added a reviewer: jeroen.dobbelaere.
brunodf added a project: clang.
brunodf requested review of this revision.
Herald added a subscriber: cfe-commits.

The invocation of a unary or binary operator for type-dependent expressions is 
represented as a CXXOperatorCallExpr. Upon template instantiation, 
TreeTransform::RebuildCXXOperatorCallExpr checks for the case of an overloaded 
operator, but not for a PseudoObject, and will directly create a UnaryOperator 
or BinaryOperator.

By invoking BuildUnaryOp or BuildBinaryOp instead, we also cover the case of 
PseudoObjects.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D111639

Files:
  clang/lib/Sema/TreeTransform.h
  clang/test/SemaCXX/PR51855.cpp

Index: clang/test/SemaCXX/PR51855.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/PR51855.cpp
@@ -0,0 +1,73 @@
+// RUN: %clang_cc1 -S -triple %itanium_abi_triple -fms-extensions -emit-llvm %s -o - | FileCheck %s
+
+struct F { };
+
+F operator*=(F& lhs, int rhs);
+
+F operator++(F& lhs);
+
+struct S {
+  short _m;
+  S(short _m): _m(_m) { }
+
+  void putM(short rhs) { _m = rhs; }
+  short getM() { return _m; }
+
+  __declspec(property(get=getM, put=putM)) short theData;
+};
+
+int test1a(int i) {
+  S tmp(i);
+  tmp.theData *= 2;
+  return tmp.theData;
+}
+
+// CHECK-LABEL: define {{.*}} i32 @_Z6test1ai(i32 %i)
+// CHECK: call void @_ZN1SC1Es
+// CHECK: call signext i16 @_ZN1S4getMEv
+// CHECK: call void @_ZN1S4putMEs
+// CHECK: call signext i16 @_ZN1S4getMEv
+
+
+template 
+int test1b(int i) {
+  T tmp(i);
+  tmp.theData *= 2;
+  return tmp.theData;
+}
+
+template int test1b(int);
+
+// CHECK-LABEL: define {{.*}} i32 @_Z6test1bI1SEii(i32 %i)
+// CHECK: call void @_ZN1SC1Es
+// CHECK: call signext i16 @_ZN1S4getMEv
+// CHECK: call void @_ZN1S4putMEs
+// CHECK: call signext i16 @_ZN1S4getMEv
+
+int test2a(int i) {
+  S tmp(i);
+  ++tmp.theData;
+  return tmp.theData;
+}
+
+// CHECK-LABEL: define {{.*}} i32 @_Z6test2ai(i32 %i)
+// CHECK: call void @_ZN1SC1Es
+// CHECK: call signext i16 @_ZN1S4getMEv
+// CHECK: call void @_ZN1S4putMEs
+// CHECK: call signext i16 @_ZN1S4getMEv
+
+template 
+int test2b(int i) {
+  T tmp(i);
+  ++tmp.theData;
+  return tmp.theData;
+}
+
+template int test2b(int);
+
+// CHECK-LABEL: define {{.*}} i32 @_Z6test2bI1SEii(i32 %i)
+// CHECK: call void @_ZN1SC1Es
+// CHECK: call signext i16 @_ZN1S4getMEv
+// CHECK: call void @_ZN1S4putMEs
+// CHECK: call signext i16 @_ZN1S4getMEv
+
Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -14605,7 +14605,8 @@
   UnaryOperatorKind Opc
 = UnaryOperator::getOverloadedOpcode(Op, isPostIncDec);
 
-  return getSema().CreateBuiltinUnaryOp(OpLoc, Opc, First);
+  // Invoke BuildUnaryOp to check for PseudoObject inc/dec
+  return getSema().BuildUnaryOp(/*Scope=*/nullptr, OpLoc, Opc, First);
 }
   } else {
 if (!First->getType()->isOverloadableType() &&
@@ -14613,8 +14614,9 @@
   // Neither of the arguments is an overloadable type, so try to
   // create a built-in binary operation.
   BinaryOperatorKind Opc = BinaryOperator::getOverloadedOpcode(Op);
+  // Invoke BuildBinOp to check for PseudoObject assignment
   ExprResult Result
-= SemaRef.CreateBuiltinBinOp(OpLoc, Opc, First, Second);
+= SemaRef.BuildBinOp(/*Scope=*/nullptr, OpLoc, Opc, First, Second);
   if (Result.isInvalid())
 return ExprError();
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111639: [Sema] check PseudoObject when rebuilding CXXOperatorCallExpr in template instantiation

[Bruno De Fraine via Phabricator via cfe-commits]

brunodf added a comment.

In D111639#3065241 , @rnk wrote:

> There are some invariants about what family of APIs TreeTransform methods 
> should call, and I've forgotten what they are, so I'm hesitant to approve 
> this. Test case looks good, though.

I am only aware of this paragraph from 
https://clang.llvm.org/docs/InternalsManual.html :

> Semantic analysis should always involve two functions: an `ActOnXXX` function 
> that will be called directly from the parser, and a `BuildXXX` function that 
> performs the actual semantic analysis and will (eventually!) build the AST 
> node. It’s fairly common for the `ActOnCXX` function to do very little (often 
> just some minor translation from the parser’s representation to Sema’s 
> representation of the same thing), but the separation is still important: C++ 
> template instantiation, for example, should always call the `BuildXXX` 
> variant.

I noticed that e.g. `TreeTransform::RebuildBinaryOperator` is also calling 
`BuildBinOp`, in line with the above description that C++ template 
instantiation should call the `BuildXXX` functions


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111639

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

[PATCH] D111639: [Sema] check PseudoObject when rebuilding CXXOperatorCallExpr in template instantiation

[Bruno De Fraine via Phabricator via cfe-commits]

brunodf updated this revision to Diff 379958.
brunodf added a comment.

Apply clang-format to diff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111639

Files:
  clang/lib/Sema/TreeTransform.h
  clang/test/SemaCXX/PR51855.cpp

Index: clang/test/SemaCXX/PR51855.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/PR51855.cpp
@@ -0,0 +1,71 @@
+// RUN: %clang_cc1 -S -triple %itanium_abi_triple -fms-extensions -emit-llvm %s -o - | FileCheck %s
+
+struct F {};
+
+F operator*=(F &lhs, int rhs);
+
+F operator++(F &lhs);
+
+struct S {
+  short _m;
+  S(short _m) : _m(_m) {}
+
+  void putM(short rhs) { _m = rhs; }
+  short getM() { return _m; }
+
+  __declspec(property(get = getM, put = putM)) short theData;
+};
+
+int test1a(int i) {
+  S tmp(i);
+  tmp.theData *= 2;
+  return tmp.theData;
+}
+
+// CHECK-LABEL: define {{.*}} i32 @_Z6test1ai(i32 %i)
+// CHECK: call void @_ZN1SC1Es
+// CHECK: call signext i16 @_ZN1S4getMEv
+// CHECK: call void @_ZN1S4putMEs
+// CHECK: call signext i16 @_ZN1S4getMEv
+
+template 
+int test1b(int i) {
+  T tmp(i);
+  tmp.theData *= 2;
+  return tmp.theData;
+}
+
+template int test1b(int);
+
+// CHECK-LABEL: define {{.*}} i32 @_Z6test1bI1SEii(i32 %i)
+// CHECK: call void @_ZN1SC1Es
+// CHECK: call signext i16 @_ZN1S4getMEv
+// CHECK: call void @_ZN1S4putMEs
+// CHECK: call signext i16 @_ZN1S4getMEv
+
+int test2a(int i) {
+  S tmp(i);
+  ++tmp.theData;
+  return tmp.theData;
+}
+
+// CHECK-LABEL: define {{.*}} i32 @_Z6test2ai(i32 %i)
+// CHECK: call void @_ZN1SC1Es
+// CHECK: call signext i16 @_ZN1S4getMEv
+// CHECK: call void @_ZN1S4putMEs
+// CHECK: call signext i16 @_ZN1S4getMEv
+
+template 
+int test2b(int i) {
+  T tmp(i);
+  ++tmp.theData;
+  return tmp.theData;
+}
+
+template int test2b(int);
+
+// CHECK-LABEL: define {{.*}} i32 @_Z6test2bI1SEii(i32 %i)
+// CHECK: call void @_ZN1SC1Es
+// CHECK: call signext i16 @_ZN1S4getMEv
+// CHECK: call void @_ZN1S4putMEs
+// CHECK: call signext i16 @_ZN1S4getMEv
Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -14605,7 +14605,8 @@
   UnaryOperatorKind Opc
 = UnaryOperator::getOverloadedOpcode(Op, isPostIncDec);
 
-  return getSema().CreateBuiltinUnaryOp(OpLoc, Opc, First);
+  // Invoke BuildUnaryOp to check for PseudoObject inc/dec
+  return getSema().BuildUnaryOp(/*Scope=*/nullptr, OpLoc, Opc, First);
 }
   } else {
 if (!First->getType()->isOverloadableType() &&
@@ -14613,8 +14614,9 @@
   // Neither of the arguments is an overloadable type, so try to
   // create a built-in binary operation.
   BinaryOperatorKind Opc = BinaryOperator::getOverloadedOpcode(Op);
-  ExprResult Result
-= SemaRef.CreateBuiltinBinOp(OpLoc, Opc, First, Second);
+  // Invoke BuildBinOp to check for PseudoObject assignment
+  ExprResult Result =
+  SemaRef.BuildBinOp(/*Scope=*/nullptr, OpLoc, Opc, First, Second);
   if (Result.isInvalid())
 return ExprError();
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111639: [Sema] check PseudoObject when rebuilding CXXOperatorCallExpr in template instantiation

[Bruno De Fraine via Phabricator via cfe-commits]

brunodf updated this revision to Diff 380885.
brunodf edited the summary of this revision.
brunodf added a reviewer: akyrtzi.
brunodf added a subscriber: akyrtzi.
brunodf added a comment.

@rnk raises the remark that TreeTransform::RebuildCXXOperatorCallExpr already 
contains code to handle ObjC pseudo objects (including checking pseudo object 
assignment) and that calling Build*Op will re-run a lot of redundant checks.

To address this, in this updated diff, I tried to generalize the code for ObjC 
pseudo object handling (which originates from @akyrtzi in commit 0f99537ecac40) 
to handle other pseudo object kinds in addition to ObjC. I think calling 
`checkPseudoObjectAssignment` and `checkPseudoObjectIncDec` is critical for the 
PR I'm trying to solve, not sure about `CheckPlaceholderExpr`, but I retained 
that from @akyrtzi his patch.

Adding @akyrtzi as reviewer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111639

Files:
  clang/lib/Sema/TreeTransform.h
  clang/test/SemaCXX/PR51855.cpp

Index: clang/test/SemaCXX/PR51855.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/PR51855.cpp
@@ -0,0 +1,71 @@
+// RUN: %clang_cc1 -S -triple %itanium_abi_triple -fms-extensions -emit-llvm %s -o - | FileCheck %s
+
+struct F {};
+
+F operator*=(F &lhs, int rhs);
+
+F operator++(F &lhs);
+
+struct S {
+  short _m;
+  S(short _m) : _m(_m) {}
+
+  void putM(short rhs) { _m = rhs; }
+  short getM() { return _m; }
+
+  __declspec(property(get = getM, put = putM)) short theData;
+};
+
+int test1a(int i) {
+  S tmp(i);
+  tmp.theData *= 2;
+  return tmp.theData;
+}
+
+// CHECK-LABEL: define {{.*}} i32 @_Z6test1ai(i32 %i)
+// CHECK: call void @_ZN1SC1Es
+// CHECK: call {{(signext )?i16}} @_ZN1S4getMEv
+// CHECK: call void @_ZN1S4putMEs
+// CHECK: call {{(signext )?i16}} @_ZN1S4getMEv
+
+template 
+int test1b(int i) {
+  T tmp(i);
+  tmp.theData *= 2;
+  return tmp.theData;
+}
+
+template int test1b(int);
+
+// CHECK-LABEL: define {{.*}} i32 @_Z6test1bI1SEii(i32 %i)
+// CHECK: call void @_ZN1SC1Es
+// CHECK: call {{(signext )?i16}} @_ZN1S4getMEv
+// CHECK: call void @_ZN1S4putMEs
+// CHECK: call {{(signext )?i16}} @_ZN1S4getMEv
+
+int test2a(int i) {
+  S tmp(i);
+  ++tmp.theData;
+  return tmp.theData;
+}
+
+// CHECK-LABEL: define {{.*}} i32 @_Z6test2ai(i32 %i)
+// CHECK: call void @_ZN1SC1Es
+// CHECK: call {{(signext )?i16}} @_ZN1S4getMEv
+// CHECK: call void @_ZN1S4putMEs
+// CHECK: call {{(signext )?i16}} @_ZN1S4getMEv
+
+template 
+int test2b(int i) {
+  T tmp(i);
+  ++tmp.theData;
+  return tmp.theData;
+}
+
+template int test2b(int);
+
+// CHECK-LABEL: define {{.*}} i32 @_Z6test2bI1SEii(i32 %i)
+// CHECK: call void @_ZN1SC1Es
+// CHECK: call {{(signext )?i16}} @_ZN1S4getMEv
+// CHECK: call void @_ZN1S4putMEs
+// CHECK: call {{(signext )?i16}} @_ZN1S4getMEv
Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -14569,24 +14569,6 @@
   Expr *Callee = OrigCallee->IgnoreParenCasts();
   bool isPostIncDec = Second && (Op == OO_PlusPlus || Op == OO_MinusMinus);
 
-  if (First->getObjectKind() == OK_ObjCProperty) {
-BinaryOperatorKind Opc = BinaryOperator::getOverloadedOpcode(Op);
-if (BinaryOperator::isAssignmentOp(Opc))
-  return SemaRef.checkPseudoObjectAssignment(/*Scope=*/nullptr, OpLoc, Opc,
- First, Second);
-ExprResult Result = SemaRef.CheckPlaceholderExpr(First);
-if (Result.isInvalid())
-  return ExprError();
-First = Result.get();
-  }
-
-  if (Second && Second->getObjectKind() == OK_ObjCProperty) {
-ExprResult Result = SemaRef.CheckPlaceholderExpr(Second);
-if (Result.isInvalid())
-  return ExprError();
-Second = Result.get();
-  }
-
   // Determine whether this should be a builtin operation.
   if (Op == OO_Subscript) {
 if (!First->getType()->isOverloadableType() &&
@@ -14602,8 +14584,21 @@
   // The argument is not of overloadable type, or this is an expression
   // of the form &Class::member, so try to create a built-in unary
   // operation.
+  //
+  // But handle pseudo-objects in the LHS, and increment and decrement
+  // of pseudo-object l-value (see Sema::BuildUnaryOp).
   UnaryOperatorKind Opc
 = UnaryOperator::getOverloadedOpcode(Op, isPostIncDec);
+  if (const BuiltinType *pty = First->getType()->getAsPlaceholderType()) {
+if (pty->getKind() == BuiltinType::PseudoObject &&
+UnaryOperator::isIncrementDecrementOp(Opc))
+  return SemaRef.checkPseudoObjectIncDec(/*Scope=*/nullptr, OpLoc, Opc,
+ First);
+ExprResult Result = SemaRef.CheckPlaceholderExpr(First);
+if (Result.isInvalid())
+  return ExprError();

[PATCH] D111639: [Sema] check PseudoObject when rebuilding CXXOperatorCallExpr in template instantiation

[Bruno De Fraine via Phabricator via cfe-commits]

brunodf updated this revision to Diff 381639.
brunodf marked 3 inline comments as done.
brunodf added a comment.

I'm adding a new patch to (partially) address the comments from @rnk.

An ObjC test case was included in the commit from @akyrtzi, I've updated it to 
also cover the case of a unary operator (and the increment/decrement), and the 
case of a type dependent on a template parameter. I moved my changes to the 
original position of the code for ObjC properties in RebuildCXXOperatorCallExpr.

With regard to OO_Subscript, OO_Arrow and OO_Amp, I have tried a number of 
things, but I have not succeeded in triggering that an CXXOperatorCallExpr is 
created for these operators (they end up as an ArraySubscriptExpr, 
CXXDependentScopeMemberExpr and UnaryOperator respectively). At the moment, I 
don't know how to test the code paths for these operators in 
RebuildCXXOperatorCallExpr.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111639

Files:
  clang/lib/Sema/TreeTransform.h
  clang/test/SemaCXX/PR51855.cpp
  clang/test/SemaObjCXX/instantiate-property-access.mm

Index: clang/test/SemaObjCXX/instantiate-property-access.mm
===
--- clang/test/SemaObjCXX/instantiate-property-access.mm
+++ clang/test/SemaObjCXX/instantiate-property-access.mm
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -DDEPENDENT -verify %s
 // expected-no-diagnostics
 
 class C {};
@@ -9,6 +10,10 @@
 
 C operator += (C c1, C c2);
 
+C operator++(C c1);
+
+bool operator!(C c1);
+
 enum TextureType { TextureType3D  };
 
 @interface Texture
@@ -16,9 +21,13 @@
 @property  C c;
 @end
 
-template  class Framebuffer {
+template  class Framebuffer {
 public:
-  Texture **color_attachment;  
+#ifdef DEPENDENT
+  T **color_attachment;
+#else
+  Texture **color_attachment;
+#endif
   Framebuffer();
 };
 
@@ -28,8 +37,15 @@
   (void)(color_attachment[0].c == color_attachment[0].c);
   (void)(color_attachment[0].c == 1);
   (void)(1 == color_attachment[0].c);
+  (void)(!color_attachment[0].textureType);
+  ++color_attachment[0].textureType;
+  (void)(!color_attachment[0].c);
 }
 
 void foo() {
+#ifdef DEPENDENT
+  Framebuffer();
+#else
   Framebuffer();
+#endif
 }
Index: clang/test/SemaCXX/PR51855.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/PR51855.cpp
@@ -0,0 +1,71 @@
+// RUN: %clang_cc1 -S -triple %itanium_abi_triple -fms-extensions -emit-llvm %s -o - | FileCheck %s
+
+struct F {};
+
+F operator*=(F &lhs, int rhs);
+
+F operator++(F &lhs);
+
+struct S {
+  short _m;
+  S(short _m) : _m(_m) {}
+
+  void putM(short rhs) { _m = rhs; }
+  short getM() { return _m; }
+
+  __declspec(property(get = getM, put = putM)) short theData;
+};
+
+int test1a(int i) {
+  S tmp(i);
+  tmp.theData *= 2;
+  return tmp.theData;
+}
+
+// CHECK-LABEL: define {{.*}} @_Z6test1ai(
+// CHECK: call {{.*}} @_ZN1SC1Es(
+// CHECK: call {{.*}} @_ZN1S4getMEv(
+// CHECK: call {{.*}} @_ZN1S4putMEs(
+// CHECK: call {{.*}} @_ZN1S4getMEv(
+
+template 
+int test1b(int i) {
+  T tmp(i);
+  tmp.theData *= 2;
+  return tmp.theData;
+}
+
+template int test1b(int);
+
+// CHECK-LABEL: define {{.*}} @_Z6test1bI1SEii(
+// CHECK: call {{.*}} @_ZN1SC1Es(
+// CHECK: call {{.*}} @_ZN1S4getMEv(
+// CHECK: call {{.*}} @_ZN1S4putMEs(
+// CHECK: call {{.*}} @_ZN1S4getMEv(
+
+int test2a(int i) {
+  S tmp(i);
+  ++tmp.theData;
+  return tmp.theData;
+}
+
+// CHECK-LABEL: define {{.*}} i32 @_Z6test2ai(
+// CHECK: call {{.*}} @_ZN1SC1Es(
+// CHECK: call {{.*}} @_ZN1S4getMEv(
+// CHECK: call {{.*}} @_ZN1S4putMEs(
+// CHECK: call {{.*}} @_ZN1S4getMEv(
+
+template 
+int test2b(int i) {
+  T tmp(i);
+  ++tmp.theData;
+  return tmp.theData;
+}
+
+template int test2b(int);
+
+// CHECK-LABEL: define {{.*}} i32 @_Z6test2bI1SEii(
+// CHECK: call void @_ZN1SC1Es(
+// CHECK: call {{.*}} @_ZN1S4getMEv(
+// CHECK: call {{.*}} @_ZN1S4putMEs(
+// CHECK: call {{.*}} @_ZN1S4getMEv(
Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -14569,18 +14569,28 @@
   Expr *Callee = OrigCallee->IgnoreParenCasts();
   bool isPostIncDec = Second && (Op == OO_PlusPlus || Op == OO_MinusMinus);
 
-  if (First->getObjectKind() == OK_ObjCProperty) {
-BinaryOperatorKind Opc = BinaryOperator::getOverloadedOpcode(Op);
-if (BinaryOperator::isAssignmentOp(Opc))
-  return SemaRef.checkPseudoObjectAssignment(/*Scope=*/nullptr, OpLoc, Opc,
- First, Second);
+  if (const BuiltinType *pty = First->getType()->getAsPlaceholderType()) {
+if (Second && !isPostIncDec) {
+  BinaryOperatorKind Opc = BinaryOperator::getOverloadedOpcode(Op);
+  if (pty->getKind() == BuiltinType::PseudoObject &&
+