[PATCH] D159126: [Clang] Add captures to the instantiation scope of lambda call operators

[Daniel Bertalan via Phabricator via cfe-commits]

BertalanD added a comment.

FYI, we found a crash related to this patch: 
https://github.com/llvm/llvm-project/issues/67260


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159126

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

[PATCH] D156156: [clang] Implement constexpr evaluation for `__builtin_{add,sub}c`

[Daniel Bertalan via Phabricator via cfe-commits]

BertalanD abandoned this revision.
BertalanD added a comment.

Moved to https://github.com/llvm/llvm-project/pull/66005


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156156

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

[PATCH] D156156: [clang] Implement constexpr evaluation for `__builtin_{add,sub}c`

[Daniel Bertalan via Phabricator via cfe-commits]

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

GCC has gained support for these multiprecision arithmetic builtins in
[`r14-1896-g2b4e0415ad6`](https://gcc.gnu.org/g:2b4e0415ad6), and although they 
aren't explicitly specified
as such in the documentation, they are usable in a constexpr context.

  

This commit adds constexpr evaluation support to Clang to match GCC's
behavior. The implementation mirrors how the builtins are lowered to a
pair of `u{add,sub}.with.overflow` operations and the carryout is set to
1 if either of those result in an overflow.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D156156

Files:
  clang/include/clang/Basic/Builtins.def
  clang/lib/AST/ExprConstant.cpp
  clang/test/SemaCXX/builtins-multiprecision.cpp

Index: clang/test/SemaCXX/builtins-multiprecision.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/builtins-multiprecision.cpp
@@ -0,0 +1,105 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++17 -verify %s
+// expected-no-diagnostics
+
+#include 
+
+template
+struct Result {
+  T value;
+  T carry;
+  constexpr bool operator==(const Result ) {
+return value == Other.value && carry == Other.carry;
+  }
+};
+
+template
+constexpr Result add(T Lhs, T Rhs, T Carryin)
+{
+  T Carryout = 0;
+  if constexpr(__is_same(T, unsigned char))
+return { __builtin_addcb(Lhs, Rhs, Carryin, ), Carryout };
+  else if constexpr(__is_same(T, unsigned short))
+return { __builtin_addcs(Lhs, Rhs, Carryin, ), Carryout };
+  else if constexpr(__is_same(T, unsigned int))
+return { __builtin_addc(Lhs, Rhs, Carryin, ), Carryout };
+  else if constexpr(__is_same(T, unsigned long))
+return { __builtin_addcl(Lhs, Rhs, Carryin, ), Carryout };
+  else if constexpr(__is_same(T, unsigned long long))
+return { __builtin_addcll(Lhs, Rhs, Carryin, ), Carryout };
+}
+
+static_assert(add(0, 0, 0) == Result{0, 0});
+static_assert(add(0, 0, 1) == Result{1, 0});
+static_assert(add(UCHAR_MAX - 1, 1, 1) == Result{0, 1});
+static_assert(add(UCHAR_MAX, 1, 0) == Result{0, 1});
+static_assert(add(UCHAR_MAX, 1, 1) == Result{1, 1});
+
+static_assert(add(0, 0, 0) == Result{0, 0});
+static_assert(add(0, 0, 1) == Result{1, 0});
+static_assert(add(USHRT_MAX - 1, 1, 1) == Result{0, 1});
+static_assert(add(USHRT_MAX, 1, 0) == Result{0, 1});
+static_assert(add(USHRT_MAX, 1, 1) == Result{1, 1});
+
+static_assert(add(0, 0, 0) == Result{0, 0});
+static_assert(add(0, 0, 1) == Result{1, 0});
+static_assert(add(UINT_MAX - 1, 1, 1) == Result{0, 1});
+static_assert(add(UINT_MAX, 1, 0) == Result{0, 1});
+static_assert(add(UINT_MAX, 1, 1) == Result{1, 1});
+
+static_assert(add(0, 0, 0) == Result{0, 0});
+static_assert(add(0, 0, 1) == Result{1, 0});
+static_assert(add(ULONG_MAX - 1, 1, 1) == Result{0, 1});
+static_assert(add(ULONG_MAX, 1, 0) == Result{0, 1});
+static_assert(add(ULONG_MAX, 1, 1) == Result{1, 1});
+
+static_assert(add(0, 0, 0) == Result{0, 0});
+static_assert(add(0, 0, 1) == Result{1, 0});
+static_assert(add(ULLONG_MAX - 1, 1, 1) == Result{0, 1});
+static_assert(add(ULLONG_MAX, 1, 0) == Result{0, 1});
+static_assert(add(ULLONG_MAX, 1, 1) == Result{1, 1});
+
+template
+constexpr Result sub(T Lhs, T Rhs, T Carryin)
+{
+  T Carryout = 0;
+  if constexpr(__is_same(T, unsigned char))
+return { __builtin_subcb(Lhs, Rhs, Carryin, ), Carryout };
+  else if constexpr(__is_same(T, unsigned short))
+return { __builtin_subcs(Lhs, Rhs, Carryin, ), Carryout };
+  else if constexpr(__is_same(T, unsigned int))
+return { __builtin_subc(Lhs, Rhs, Carryin, ), Carryout };
+  else if constexpr(__is_same(T, unsigned long))
+return { __builtin_subcl(Lhs, Rhs, Carryin, ), Carryout };
+  else if constexpr(__is_same(T, unsigned long long))
+return { __builtin_subcll(Lhs, Rhs, Carryin, ), Carryout };
+}
+
+static_assert(sub(0, 0, 0) == Result{0, 0});
+static_assert(sub(0, 0, 1) == Result{UCHAR_MAX, 1});
+static_assert(sub(0, 1, 0) == Result{UCHAR_MAX, 1});
+static_assert(sub(0, 1, 1) == Result{UCHAR_MAX - 1, 1});
+static_assert(sub(1, 0, 0) == Result{1, 0});
+
+static_assert(sub(0, 0, 0) == Result{0, 0});
+static_assert(sub(0, 0, 1) == Result{USHRT_MAX, 1});
+static_assert(sub(0, 1, 0) == Result{USHRT_MAX, 1});
+static_assert(sub(0, 1, 1) == Result{USHRT_MAX - 1, 1});
+static_assert(sub(1, 0, 0) == Result{1, 0});
+
+static_assert(sub(0, 0, 0) == Result{0, 0});
+static_assert(sub(0, 0, 1) == Result{UINT_MAX, 1});
+static_assert(sub(0, 1, 0) == Result{UINT_MAX, 1});
+static_assert(sub(0, 1, 1) == Result{UINT_MAX - 1, 1});
+static_assert(sub(1, 0, 0) == Result{1, 0});
+
+static_assert(sub(0, 0, 0) == Result{0, 0});
+static_assert(sub(0, 0, 1) == Result{ULONG_MAX, 1});
+static_assert(sub(0, 1, 0) == Result{ULONG_MAX, 1});
+static_assert(sub(0, 1, 1) 

[PATCH] D154397: [Driver] Default to -ftls-model=initial-exec on SerenityOS

[Daniel Bertalan via Phabricator via cfe-commits]

BertalanD added a comment.

Hi MaskRay!

Your blog post (alongside the Drepper TLS doc) proved to be a very helpful 
resource for our implementation of the dynamic TLS interface. I currently have 
a SerenityOS PR  open that 
implements `__tls_get_addr` and TLSDESC on top of our current TLS 
infrastructure which only supports static TLS blocks. (Long story short, our 
loader passes the initial TLS image to the kernel, which automagically copies 
it over and sets up the thread pointer whenever a new thread is created. We 
need to move it to the userspace if we want to have a dynamic thread vector and 
separately allocated TLS blocks). I'd love it if you'd take a look at the PR.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154397

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

[PATCH] D151952: [clang] adds `__type_pack_index` so we can get a type's parameter pack index

[Daniel Bertalan via Phabricator via cfe-commits]

BertalanD added a subscriber: jwakely.
BertalanD added a comment.

I am considering making use of this feature in a real-world project. I have a 
few short observations after rebasing this diff and playing with it for a bit:

- It seems to me that `__has_builtin(__type_pack_index)`  currently evaluates 
to false. Please include a check for the feature detection working correctly in 
the test.

- Has there been any coordination with GCC folks on the name/semantics of this 
builtin?

  GCC trunk has recently gained support for `__type_pack_element`, and on the 
feature request for it (PR100157 ), @jwakely has 
already expressed interest in implementing the reverse of that, i.e. what this 
diff proposes. A slight incompatibility has managed to seep in: GCC does not 
allow the use of `__type_pack_element` in a position where its name would end 
up being manged; see the Folly bug report 
. While this is easy to work 
around (wrap it in a template), as a user, it would be inconvenient needing to 
add a GCC/Clang check alongside `__has_builtin` if the two compilers were to 
come up with incompatibly behaving implementations for this one.

- What would be the expected way of gracefully handing the queried type not 
being present in the list?

  Our `std::variant`-like type uses a `can_contain` constraint on its accessors 
to prevent asking for a type that's not in the type list. We currently achieve 
this by `index_of` returning `sizeof...(Ts)` as a sentinel value if no match 
was found. libc++'s __find_exactly_one_t 

 uses `(size_t)-1` for the same purpose.

  I'm targeting C++20, so `requires { __type_pack_index(T, InTypes...); }` 
seems to do just what I want, but `std::variant` is C++17 and `std::tuple` is 
even earlier than that. Of course, in earlier language modes you don't have to 
worry about it working in concepts, but e.g. libc++ `static_assert`s that the 
index is not the sentinel. Not sure about what libstdc++'s requirements would 
be.

  In any case, could you please test/document the expected way of recovering 
from the error?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151952

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

[PATCH] D134529: [C++20][Clang] P2468R2 The Equality Operator You Are Looking For

[Daniel Bertalan via Phabricator via cfe-commits]

BertalanD added a comment.

Thank you all for the help and clarification! I'll go fix our code then.

As an aside, I understand that this paper now retroactively applies to the C++ 
standard, but code has already been written that assumes that what my example 
is doing is valid. Does it deserve a mention in the Potentially Breaking 
Changes section of the release notes?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134529

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Daniel Bertalan via Phabricator via cfe-commits]

BertalanD added a comment.

(I know this is a very contrived example with `void operator!=`, but that is 
what CVise spat out, and the actual failures are related to comparison 
operators too)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Daniel Bertalan via Phabricator via cfe-commits]

BertalanD added a comment.

I tried out D135772 , and our build got 
significantly farther than before! I unfortunately discovered another piece of 
code that pre babdef27c503c0bbbcc017e9f88affddda90ea4e 
 Clang and 
GCC accept in C++20 mode, but Clang trunk does not 
(https://godbolt.org/z/q1q4nfobK):

  struct String {
String(char *);
bool operator==(String const &);
void operator!=(String const &);
  };
  
  extern char* c;
  extern String s;
  int test() {
return c == s;
  }



  :10:12: error: invalid operands to binary expression ('char *' and 
'String')
return c == s;
   ~ ^  ~


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Daniel Bertalan via Phabricator via cfe-commits]

BertalanD added a comment.

In D126907#3849583 , @erichkeane 
wrote:

> In D126907#3846591 , @erichkeane 
> wrote:
>
>> In D126907#3844788 , @BertalanD 
>> wrote:
>>
>>> Hi @erichkeane,
>>>
>>> This change broke compilation of this program 
>>> (https://godbolt.org/z/KrWGvcf8h; reduced from 
>>> https://github.com/SerenityOS/ladybird):
>>>
>>>   template
>>>   constexpr bool IsSame = false;
>>>   
>>>   template
>>>   constexpr bool IsSame = true;
>>>   
>>>   template
>>>   struct Foo {
>>>   template
>>>   Foo(U&&) requires (!IsSame);
>>>   };
>>>   
>>>   template<>
>>>   struct Foo : Foo {
>>>   using Foo::Foo;
>>>   };
>>>   
>>>   Foo test() { return 0; }
>>>
>>>
>>>
>>>   :18:27: error: invalid reference to function 'Foo': constraints 
>>> not satisfied
>>>   Foo test() { return 0; }
>>> ^
>>>   :10:24: note: because substituted constraint expression is 
>>> ill-formed: value of type '' is not contextually 
>>> convertible to 'bool'
>>>   Foo(U&&) requires (!IsSame);
>>>  ^
>>
>> Thanks for the report!  I'll look into it ASAP.
>
> Quick note: I believe I understand the cause of this, which requires a bit 
> more work than I otherwise would have expected.  I have a candidate patch I'm 
> running through my testing right now that should fix this, but it still needs 
> cleaning up.  Expect it in the next day or two if all goes well.

Thank you for your quick response and for creating this massive yak shave of a 
patch :D

Please ping me if you want me to test the fix on our code, or if I can help in 
some other way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

[Daniel Bertalan via Phabricator via cfe-commits]

BertalanD added a comment.

Hi @erichkeane,

This change broke compilation of this program (https://godbolt.org/z/KrWGvcf8h; 
reduced from https://github.com/SerenityOS/ladybird):

  template
  constexpr bool IsSame = false;
  
  template
  constexpr bool IsSame = true;
  
  template
  struct Foo {
  template
  Foo(U&&) requires (!IsSame);
  };
  
  template<>
  struct Foo : Foo {
  using Foo::Foo;
  };
  
  Foo test() { return 0; }



  :18:27: error: invalid reference to function 'Foo': constraints not 
satisfied
  Foo test() { return 0; }
^
  :10:24: note: because substituted constraint expression is 
ill-formed: value of type '' is not contextually convertible to 
'bool'
  Foo(U&&) requires (!IsSame);
 ^


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126907

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

[PATCH] D130689: [LLVM] Update C++ standard to 17

[Daniel Bertalan via Phabricator via cfe-commits]

BertalanD added inline comments.



Comment at: llvm/cmake/modules/CheckCompilerVersion.cmake:79-80
 set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} -std=c++0x")
 # Test for libstdc++ version of at least 4.8 by checking for 
_ZNKSt17bad_function_call4whatEv.
 # Note: We should check _GLIBCXX_RELEASE when possible (i.e., for GCC 7.1 
and up).
 check_cxx_source_compiles("

This comment should be updated too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130689

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

[PATCH] D124736: [CodeGen] Use ABI alignment for placement new

[Daniel Bertalan via Phabricator via cfe-commits]

BertalanD added a comment.

Could you please commit this for me, @rjmccall?

I'm wondering if there's a chance this could be backported to the LLVM 14 
branch.  We've been running into the linked issue in production, it would be 
great if it were fixed in the next point release.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124736

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

[PATCH] D124736: [CodeGen] Use ABI alignment for placement new

[Daniel Bertalan via Phabricator via cfe-commits]

BertalanD updated this revision to Diff 428225.
BertalanD added a comment.

Use ABI alignment for allocating operator new as well.

Sorry for the delay.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124736

Files:
  clang/lib/CodeGen/CGExprCXX.cpp
  clang/test/CodeGenCXX/pr54845.cpp


Index: clang/test/CodeGenCXX/pr54845.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/pr54845.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -triple i686-linux-gnu -emit-llvm %s -o - | FileCheck %s
+// https://github.com/llvm/llvm-project/issues/54845
+
+void *operator new(unsigned int, void *);
+
+void test(double *d) {
+  // This store used to have an alignment of 8, which was incorrect as
+  // the i386 psABI only guarantees a 4-byte alignment for doubles.
+
+  // CHECK: store double 0.00e+00, {{.*}}, align 4
+  new (d) double(0);
+}
Index: clang/lib/CodeGen/CGExprCXX.cpp
===
--- clang/lib/CodeGen/CGExprCXX.cpp
+++ clang/lib/CodeGen/CGExprCXX.cpp
@@ -1573,7 +1573,7 @@
   llvm::Value *allocSize =
 EmitCXXNewAllocSize(*this, E, minElements, numElements,
 allocSizeWithoutCookie);
-  CharUnits allocAlign = getContext().getPreferredTypeAlignInChars(allocType);
+  CharUnits allocAlign = getContext().getTypeAlignInChars(allocType);
 
   // Emit the allocation call.  If the allocator is a global placement
   // operator, just "inline" it directly.


Index: clang/test/CodeGenCXX/pr54845.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/pr54845.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -triple i686-linux-gnu -emit-llvm %s -o - | FileCheck %s
+// https://github.com/llvm/llvm-project/issues/54845
+
+void *operator new(unsigned int, void *);
+
+void test(double *d) {
+  // This store used to have an alignment of 8, which was incorrect as
+  // the i386 psABI only guarantees a 4-byte alignment for doubles.
+
+  // CHECK: store double 0.00e+00, {{.*}}, align 4
+  new (d) double(0);
+}
Index: clang/lib/CodeGen/CGExprCXX.cpp
===
--- clang/lib/CodeGen/CGExprCXX.cpp
+++ clang/lib/CodeGen/CGExprCXX.cpp
@@ -1573,7 +1573,7 @@
   llvm::Value *allocSize =
 EmitCXXNewAllocSize(*this, E, minElements, numElements,
 allocSizeWithoutCookie);
-  CharUnits allocAlign = getContext().getPreferredTypeAlignInChars(allocType);
+  CharUnits allocAlign = getContext().getTypeAlignInChars(allocType);
 
   // Emit the allocation call.  If the allocator is a global placement
   // operator, just "inline" it directly.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124736: [CodeGen] Use ABI alignment for placement new

[Daniel Bertalan via Phabricator via cfe-commits]

BertalanD added a comment.

In D124736#3485128 , @rjmccall wrote:

> It should probably be the ABI alignment in all cases; I think the preferred 
> alignment is just supposed to be used to opportunistically over-align things 
> like e.g. local variables, which doesn't seem relevant for the ABI code 
> involving a call to `operator new`.

Right, that makes sense. I'll update this patch tomorrow. Thanks for the review 
:)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124736

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

[PATCH] D124736: [CodeGen] Use ABI alignment for placement new

[Daniel Bertalan via Phabricator via cfe-commits]

BertalanD updated this revision to Diff 426303.
BertalanD added a comment.

Removed accidental execute permission from the test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124736

Files:
  clang/lib/CodeGen/CGExprCXX.cpp
  clang/test/CodeGenCXX/pr54845.cpp


Index: clang/test/CodeGenCXX/pr54845.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/pr54845.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -triple i686-linux-gnu -emit-llvm %s -o - | FileCheck %s
+// https://github.com/llvm/llvm-project/issues/54845
+
+void *operator new(unsigned int, void *);
+
+void test(double *d) {
+  // This store used to have an alignment of 8, which was incorrect as
+  // the i386 psABI only guarantees a 4-byte alignment for doubles.
+
+  // CHECK: store double 0.00e+00, {{.*}}, align 4
+  new (d) double(0);
+}
Index: clang/lib/CodeGen/CGExprCXX.cpp
===
--- clang/lib/CodeGen/CGExprCXX.cpp
+++ clang/lib/CodeGen/CGExprCXX.cpp
@@ -1573,7 +1573,7 @@
   llvm::Value *allocSize =
 EmitCXXNewAllocSize(*this, E, minElements, numElements,
 allocSizeWithoutCookie);
-  CharUnits allocAlign = getContext().getPreferredTypeAlignInChars(allocType);
+  CharUnits allocAlign;
 
   // Emit the allocation call.  If the allocator is a global placement
   // operator, just "inline" it directly.
@@ -1583,6 +1583,8 @@
 assert(E->getNumPlacementArgs() == 1);
 const Expr *arg = *E->placement_arguments().begin();
 
+allocAlign = getContext().getTypeAlignInChars(allocType);
+
 LValueBaseInfo BaseInfo;
 allocation = EmitPointerWithAlignment(arg, );
 
@@ -1605,6 +1607,8 @@
   allocator->getType()->castAs();
 unsigned ParamsToSkip = 0;
 
+allocAlign = getContext().getPreferredTypeAlignInChars(allocType);
+
 // The allocation size is the first argument.
 QualType sizeType = getContext().getSizeType();
 allocatorArgs.add(RValue::get(allocSize), sizeType);


Index: clang/test/CodeGenCXX/pr54845.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/pr54845.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -triple i686-linux-gnu -emit-llvm %s -o - | FileCheck %s
+// https://github.com/llvm/llvm-project/issues/54845
+
+void *operator new(unsigned int, void *);
+
+void test(double *d) {
+  // This store used to have an alignment of 8, which was incorrect as
+  // the i386 psABI only guarantees a 4-byte alignment for doubles.
+
+  // CHECK: store double 0.00e+00, {{.*}}, align 4
+  new (d) double(0);
+}
Index: clang/lib/CodeGen/CGExprCXX.cpp
===
--- clang/lib/CodeGen/CGExprCXX.cpp
+++ clang/lib/CodeGen/CGExprCXX.cpp
@@ -1573,7 +1573,7 @@
   llvm::Value *allocSize =
 EmitCXXNewAllocSize(*this, E, minElements, numElements,
 allocSizeWithoutCookie);
-  CharUnits allocAlign = getContext().getPreferredTypeAlignInChars(allocType);
+  CharUnits allocAlign;
 
   // Emit the allocation call.  If the allocator is a global placement
   // operator, just "inline" it directly.
@@ -1583,6 +1583,8 @@
 assert(E->getNumPlacementArgs() == 1);
 const Expr *arg = *E->placement_arguments().begin();
 
+allocAlign = getContext().getTypeAlignInChars(allocType);
+
 LValueBaseInfo BaseInfo;
 allocation = EmitPointerWithAlignment(arg, );
 
@@ -1605,6 +1607,8 @@
   allocator->getType()->castAs();
 unsigned ParamsToSkip = 0;
 
+allocAlign = getContext().getPreferredTypeAlignInChars(allocType);
+
 // The allocation size is the first argument.
 QualType sizeType = getContext().getSizeType();
 allocatorArgs.add(RValue::get(allocSize), sizeType);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124736: [CodeGen] Use ABI alignment for placement new

[Daniel Bertalan via Phabricator via cfe-commits]

BertalanD updated this revision to Diff 426301.
BertalanD added a comment.

Added a test case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124736

Files:
  clang/lib/CodeGen/CGExprCXX.cpp
  clang/test/CodeGenCXX/pr54845.cpp


Index: clang/test/CodeGenCXX/pr54845.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/pr54845.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -triple i686-linux-gnu -emit-llvm %s -o - | FileCheck %s
+// https://github.com/llvm/llvm-project/issues/54845
+
+void *operator new(unsigned int, void *);
+
+void test(double *d) {
+  // This store used to have an alignment of 8, which was incorrect as
+  // the i386 psABI only guarantees a 4-byte alignment for doubles.
+
+  // CHECK: store double 0.00e+00, {{.*}}, align 4
+  new (d) double(0);
+}
Index: clang/lib/CodeGen/CGExprCXX.cpp
===
--- clang/lib/CodeGen/CGExprCXX.cpp
+++ clang/lib/CodeGen/CGExprCXX.cpp
@@ -1573,7 +1573,7 @@
   llvm::Value *allocSize =
 EmitCXXNewAllocSize(*this, E, minElements, numElements,
 allocSizeWithoutCookie);
-  CharUnits allocAlign = getContext().getPreferredTypeAlignInChars(allocType);
+  CharUnits allocAlign;
 
   // Emit the allocation call.  If the allocator is a global placement
   // operator, just "inline" it directly.
@@ -1583,6 +1583,8 @@
 assert(E->getNumPlacementArgs() == 1);
 const Expr *arg = *E->placement_arguments().begin();
 
+allocAlign = getContext().getTypeAlignInChars(allocType);
+
 LValueBaseInfo BaseInfo;
 allocation = EmitPointerWithAlignment(arg, );
 
@@ -1605,6 +1607,8 @@
   allocator->getType()->castAs();
 unsigned ParamsToSkip = 0;
 
+allocAlign = getContext().getPreferredTypeAlignInChars(allocType);
+
 // The allocation size is the first argument.
 QualType sizeType = getContext().getSizeType();
 allocatorArgs.add(RValue::get(allocSize), sizeType);


Index: clang/test/CodeGenCXX/pr54845.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/pr54845.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -triple i686-linux-gnu -emit-llvm %s -o - | FileCheck %s
+// https://github.com/llvm/llvm-project/issues/54845
+
+void *operator new(unsigned int, void *);
+
+void test(double *d) {
+  // This store used to have an alignment of 8, which was incorrect as
+  // the i386 psABI only guarantees a 4-byte alignment for doubles.
+
+  // CHECK: store double 0.00e+00, {{.*}}, align 4
+  new (d) double(0);
+}
Index: clang/lib/CodeGen/CGExprCXX.cpp
===
--- clang/lib/CodeGen/CGExprCXX.cpp
+++ clang/lib/CodeGen/CGExprCXX.cpp
@@ -1573,7 +1573,7 @@
   llvm::Value *allocSize =
 EmitCXXNewAllocSize(*this, E, minElements, numElements,
 allocSizeWithoutCookie);
-  CharUnits allocAlign = getContext().getPreferredTypeAlignInChars(allocType);
+  CharUnits allocAlign;
 
   // Emit the allocation call.  If the allocator is a global placement
   // operator, just "inline" it directly.
@@ -1583,6 +1583,8 @@
 assert(E->getNumPlacementArgs() == 1);
 const Expr *arg = *E->placement_arguments().begin();
 
+allocAlign = getContext().getTypeAlignInChars(allocType);
+
 LValueBaseInfo BaseInfo;
 allocation = EmitPointerWithAlignment(arg, );
 
@@ -1605,6 +1607,8 @@
   allocator->getType()->castAs();
 unsigned ParamsToSkip = 0;
 
+allocAlign = getContext().getPreferredTypeAlignInChars(allocType);
+
 // The allocation size is the first argument.
 QualType sizeType = getContext().getSizeType();
 allocatorArgs.add(RValue::get(allocSize), sizeType);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124736: [CodeGen] Use ABI alignment for placement new

[Daniel Bertalan via Phabricator via cfe-commits]

BertalanD created this revision.
Herald added a project: All.
BertalanD requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

If we do not know the alignment of the operand, we can't assume it has
the preferred alignment. It might be e.g. a pointer to a struct member
which follows ABI alignment rules.

This makes UBSAN no longer report "constructor call on misaligned
address" when constructing a double into a struct field of type double
on i686. The psABI specifies an alignment of 4 bytes, but the preferred
alignment used by Clang is 8 bytes.

Fixes #54845


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D124736

Files:
  clang/lib/CodeGen/CGExprCXX.cpp


Index: clang/lib/CodeGen/CGExprCXX.cpp
===
--- clang/lib/CodeGen/CGExprCXX.cpp
+++ clang/lib/CodeGen/CGExprCXX.cpp
@@ -1573,7 +1573,7 @@
   llvm::Value *allocSize =
 EmitCXXNewAllocSize(*this, E, minElements, numElements,
 allocSizeWithoutCookie);
-  CharUnits allocAlign = getContext().getPreferredTypeAlignInChars(allocType);
+  CharUnits allocAlign;
 
   // Emit the allocation call.  If the allocator is a global placement
   // operator, just "inline" it directly.
@@ -1583,6 +1583,8 @@
 assert(E->getNumPlacementArgs() == 1);
 const Expr *arg = *E->placement_arguments().begin();
 
+allocAlign = getContext().getTypeAlignInChars(allocType);
+
 LValueBaseInfo BaseInfo;
 allocation = EmitPointerWithAlignment(arg, );
 
@@ -1605,6 +1607,8 @@
   allocator->getType()->castAs();
 unsigned ParamsToSkip = 0;
 
+allocAlign = getContext().getPreferredTypeAlignInChars(allocType);
+
 // The allocation size is the first argument.
 QualType sizeType = getContext().getSizeType();
 allocatorArgs.add(RValue::get(allocSize), sizeType);


Index: clang/lib/CodeGen/CGExprCXX.cpp
===
--- clang/lib/CodeGen/CGExprCXX.cpp
+++ clang/lib/CodeGen/CGExprCXX.cpp
@@ -1573,7 +1573,7 @@
   llvm::Value *allocSize =
 EmitCXXNewAllocSize(*this, E, minElements, numElements,
 allocSizeWithoutCookie);
-  CharUnits allocAlign = getContext().getPreferredTypeAlignInChars(allocType);
+  CharUnits allocAlign;
 
   // Emit the allocation call.  If the allocator is a global placement
   // operator, just "inline" it directly.
@@ -1583,6 +1583,8 @@
 assert(E->getNumPlacementArgs() == 1);
 const Expr *arg = *E->placement_arguments().begin();
 
+allocAlign = getContext().getTypeAlignInChars(allocType);
+
 LValueBaseInfo BaseInfo;
 allocation = EmitPointerWithAlignment(arg, );
 
@@ -1605,6 +1607,8 @@
   allocator->getType()->castAs();
 unsigned ParamsToSkip = 0;
 
+allocAlign = getContext().getPreferredTypeAlignInChars(allocType);
+
 // The allocation size is the first argument.
 QualType sizeType = getContext().getSizeType();
 allocatorArgs.add(RValue::get(allocSize), sizeType);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits