[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.
Herald added a subscriber: StephenFan.
Herald added a project: All.

Are we considering relanding this with smaller alignment for smaller mallocs 
(and friends)?

See: https://github.com/llvm/llvm-project/issues/54747#issuecomment-1088420860


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118804

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

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

[James Y Knight via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG9545976ff160: Revert [Clang] Propagate guaranteed 
alignment for malloc and others (authored by jyknight).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118804

Files:
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CodeGen/alloc-fns-alignment.c

Index: clang/test/CodeGen/alloc-fns-alignment.c
===
--- clang/test/CodeGen/alloc-fns-alignment.c
+++ clang/test/CodeGen/alloc-fns-alignment.c
@@ -1,12 +1,9 @@
-// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm < %s | FileCheck %s --check-prefix=ALIGN16
-// RUN: %clang_cc1 -triple x86_64-windows-msvc  -emit-llvm < %s | FileCheck %s --check-prefix=ALIGN16
-// RUN: %clang_cc1 -triple i386-apple-darwin-emit-llvm < %s | FileCheck %s --check-prefix=ALIGN16
-// RUN: %clang_cc1 -triple i386-unknown-linux-gnu   -emit-llvm < %s | FileCheck %s --check-prefix=ALIGN8
-// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fno-builtin-malloc  -emit-llvm < %s  | FileCheck %s --check-prefix=NOBUILTIN-MALLOC
-// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fno-builtin-calloc  -emit-llvm < %s  | FileCheck %s --check-prefix=NOBUILTIN-CALLOC
-// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fno-builtin-realloc -emit-llvm < %s  | FileCheck %s --check-prefix=NOBUILTIN-REALLOC
-// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fno-builtin-aligned_alloc -emit-llvm < %s  | FileCheck %s --check-prefix=NOBUILTIN-ALIGNED_ALLOC
-// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fno-builtin-memalign -emit-llvm < %s  | FileCheck %s --check-prefix=NOBUILTIN-MEMALIGN
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm < %s | FileCheck %s
+
+// Note: this test originally asserted that malloc/calloc/realloc got alignment
+// attributes on their return pointer. However, that was reverted in
+// https://reviews.llvm.org/D118804 and it now asserts that they do _NOT_ get
+// align attributes.
 
 typedef __SIZE_TYPE__ size_t;
 
@@ -49,57 +46,36 @@
 }
 
 // CHECK-LABEL: @malloc_test
-// ALIGN16: align 16 i8* @malloc
-
-// CHECK-LABEL: @calloc_test
-// ALIGN16: align 16 i8* @calloc
-
-// CHECK-LABEL: @realloc_test
-// ALIGN16: align 16 i8* @realloc
+// CHECK: call i8* @malloc
 
-// CHECK-LABEL: @aligned_alloc_variable_test
-// ALIGN16:  %[[ALLOCATED:.*]] = call align 16 i8* @aligned_alloc({{i32|i64}} noundef %[[ALIGN:.*]], {{i32|i64}} noundef %[[NBYTES:.*]])
-// ALIGN16-NEXT: call void @llvm.assume(i1 true) [ "align"(i8* %[[ALLOCATED]], {{i32|i64}} %[[ALIGN]]) ]
-
-// CHECK-LABEL: @memalign_variable_test
-// ALIGN16:  %[[ALLOCATED:.*]] = call align 16 i8* @memalign({{i32|i64}} noundef %[[ALIGN:.*]], {{i32|i64}} noundef %[[NBYTES:.*]])
-// ALIGN16-NEXT: call void @llvm.assume(i1 true) [ "align"(i8* %[[ALLOCATED]], {{i32|i64}} %[[ALIGN]]) ]
-
-// CHECK-LABEL: @aligned_alloc_constant_test
-// ALIGN16: align 16 i8* @aligned_alloc
-
-// CHECK-LABEL: @aligned_alloc_large_constant_test
-// ALIGN16: align 4096 i8* @aligned_alloc
-
-// CHECK-LABEL: @memalign_large_constant_test
-// ALIGN16: align 4096 i8* @memalign
-
-// CHECK-LABEL: @malloc_test
-// ALIGN8: align 8 i8* @malloc
+// CHECK: declare i8* @malloc
 
 // CHECK-LABEL: @calloc_test
-// ALIGN8: align 8 i8* @calloc
+// CHECK: call i8* @calloc
+
+// CHECK: declare i8* @calloc
 
 // CHECK-LABEL: @realloc_test
-// ALIGN8: align 8 i8* @realloc
+// CHECK: call i8* @realloc
+
+// CHECK: declare i8* @realloc
 
 // CHECK-LABEL: @aligned_alloc_variable_test
-// ALIGN8: align 8 i8* @aligned_alloc
+// CHECK:  %[[ALLOCATED:.*]] = call i8* @aligned_alloc({{i32|i64}} noundef %[[ALIGN:.*]], {{i32|i64}} noundef %[[NBYTES:.*]])
+// CHECK-NEXT: call void @llvm.assume(i1 true) [ "align"(i8* %[[ALLOCATED]], {{i32|i64}} %[[ALIGN]]) ]
+
+// CHECK: declare i8* @aligned_alloc
 
 // CHECK-LABEL: @memalign_variable_test
-// ALIGN8: align 8 i8* @memalign
+// CHECK:  %[[ALLOCATED:.*]] = call i8* @memalign({{i32|i64}} noundef %[[ALIGN:.*]], {{i32|i64}} noundef %[[NBYTES:.*]])
+// CHECK-NEXT: call void @llvm.assume(i1 true) [ "align"(i8* %[[ALLOCATED]], {{i32|i64}} %[[ALIGN]]) ]
 
 // CHECK-LABEL: @aligned_alloc_constant_test
-// ALIGN8: align 8 i8* @aligned_alloc
+// CHECK: call align 8 i8* @aligned_alloc
 
 // CHECK-LABEL: @aligned_alloc_large_constant_test
-// ALIGN8: align 4096 i8* @aligned_alloc
+// CHECK: call align 4096 i8* @aligned_alloc
 
 // CHECK-LABEL: @memalign_large_constant_test
-// ALIGN8: align 4096 i8* @memalign
+// CHECK: align 4096 i8* @memalign
 
-// NOBUILTIN-MALLOC: declare i8* @malloc
-// NOBUILTIN-CALLOC: declare i8* @calloc
-// NOBUILTIN-REALLOC: declare i8* @realloc
-// NOBUILTIN-ALIGNED_ALLOC: declare i8* @aligned_alloc
-// NOBUILTIN-MEMALIGN: declare i8* @memalign

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D118804#3304261 , @urnathan wrote:

> While C2X has blessed such smaller alignments, the x86_64 ABI (in 
> particular), has not.  However, using that ABI to justify 'It. Is. 16. 
> Bytes.', is really an exercise in reality denial at this point.  just thought 
> I'd make it clear we have conflicting standards and practicality to attend to.

By x86-64 ABI, do you mean "System V Application Binary Interface AMD64 
Architecture Processor Supplement"? I have searched a bit but do not find the 
relevant wording on memory allocation.
If there is one, you can report issues on 
https://gitlab.com/x86-psABIs/x86-64-ABI/-/issues (gitlab account needed).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118804

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

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

I'm not sure your new wording is any clearer; a

In D118804#3304337 , @urnathan wrote:

> In D118804#3304280 , @aaron.ballman 
> wrote:
>
>> In D118804#3304261 , @urnathan 
>> wrote:
>>
>>> While C2X has blessed such smaller alignments, the x86_64 ABI (in 
>>> particular), has not.  However, using that ABI to justify 'It. Is. 16. 
>>> Bytes.', is really an exercise in reality denial at this point.  just 
>>> thought I'd make it clear we have conflicting standards and practicality to 
>>> attend to.
>>
>> Do we want me to report back to WG14 with information that N2293 might not 
>> suitable for adoption into C2x?
>
> I think N2293 is fine for C2x.  It is blessing an implementation of lower 
> alignment allocations.  Putting the programmer on notice that they can no 
> longer assume some things.
>
> As a compiler I think we need to deal with the reality that there are non-ABI 
> conforming [system-dependent] allocators out there, and not simply say 'But 
> the ABI says ...'

If a platform's ABI guarantees something, and the platform's system allocator 
actually does it, then that's the required behavior there, and replacement 
allocators that don't live up to it are non-conformant.  But if the system 
allocator intentionally *doesn't* do it, and the platform's ABI is a generic 
ABI, then yeah, we have to understand that as an intent to declare a 
platform-specific deviation from the generic ABI.

> There is already at least one thing the ABI says that is not valid on some 
> systems [sret return behaviour and Swift], for equally good reasons.  The 
> compiler deals with that.

Can you explain what you mean here?  Swift's conventions are not really bound 
by the C ABI.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118804

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

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D118804#3304337 , @urnathan wrote:

> In D118804#3304280 , @aaron.ballman 
> wrote:
>
>> In D118804#3304261 , @urnathan 
>> wrote:
>>
>>> While C2X has blessed such smaller alignments, the x86_64 ABI (in 
>>> particular), has not.  However, using that ABI to justify 'It. Is. 16. 
>>> Bytes.', is really an exercise in reality denial at this point.  just 
>>> thought I'd make it clear we have conflicting standards and practicality to 
>>> attend to.
>>
>> Do we want me to report back to WG14 with information that N2293 might not 
>> suitable for adoption into C2x?
>
> I think N2293 is fine for C2x.  It is blessing an implementation of lower 
> alignment allocations.  Putting the programmer on notice that they can no 
> longer assume some things.

Okay, that's good to know, thank you! If the consensus changes and we want me 
to take feedback to WG14, please let me know.

> As a compiler I think we need to deal with the reality that there are non-ABI 
> conforming [system-dependent] allocators out there, and not simply say 'But 
> the ABI says ...'
>
> There is already at least one thing the ABI says that is not valid on some 
> systems [sret return behaviour and Swift], for equally good reasons.  The 
> compiler deals with that.
>
> FWIW, although gcc's code generation has a similar bug, its C++ library is 
> already cognizant of lower-alignment allocators -- I convinced Mr Wakely a 
> few years ago :)




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118804

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

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

>> I think we need to deal with the reality that there are non-ABI conforming 
>> [system-dependent] allocators out there,

Definitely not a good thing, just ticking bomb.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118804

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

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

[Nathan Sidwell via Phabricator via cfe-commits]

urnathan added a comment.

In D118804#3304280 , @aaron.ballman 
wrote:

> In D118804#3304261 , @urnathan 
> wrote:
>
>> While C2X has blessed such smaller alignments, the x86_64 ABI (in 
>> particular), has not.  However, using that ABI to justify 'It. Is. 16. 
>> Bytes.', is really an exercise in reality denial at this point.  just 
>> thought I'd make it clear we have conflicting standards and practicality to 
>> attend to.
>
> Do we want me to report back to WG14 with information that N2293 might not 
> suitable for adoption into C2x?

I think N2293 is fine for C2x.  It is blessing an implementation of lower 
alignment allocations.  Putting the programmer on notice that they can no 
longer assume some things.

As a compiler I think we need to deal with the reality that there are non-ABI 
conforming [system-dependent] allocators out there, and not simply say 'But the 
ABI says ...'

There is already at least one thing the ABI says that is not valid on some 
systems [sret return behaviour and Swift], for equally good reasons.  The 
compiler deals with that.

FWIW, although gcc's code generation has a similar bug, its C++ library is 
already cognizant of lower-alignment allocators -- I convinced Mr Wakely a few 
years ago :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118804

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

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D118804#3304261 , @urnathan wrote:

> While C2X has blessed such smaller alignments, the x86_64 ABI (in 
> particular), has not.  However, using that ABI to justify 'It. Is. 16. 
> Bytes.', is really an exercise in reality denial at this point.  just thought 
> I'd make it clear we have conflicting standards and practicality to attend to.

Do we want me to report back to WG14 with information that N2293 might not 
suitable for adoption into C2x?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118804

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

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

[Nathan Sidwell via Phabricator via cfe-commits]

urnathan added a comment.

While C2X has blessed such smaller alignments, the x86_64 ABI (in particular), 
has not.  However, using that ABI to justify 'It. Is. 16. Bytes.', is really an 
exercise in reality denial at this point.  just thought I'd make it clear we 
have conflicting standards and practicality to attend to.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118804

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

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D118804#3303004 , @jyknight wrote:

> In D118804#3302675 , @rsmith wrote:
>
>> I support this revert.
>
> Having received enough support, I'll go ahead and commit, and then propose 
> backport to llvm 14 branch.
>
> But --
>
>> - `malloc` always returns storage whose alignment is at least the largest 
>> fundamental alignment.
>
> As has been discussed previously in this review thread, that's not true -- 
> and the in-practice-falseness of this statement was the trigger for reverting 
> the change.
>
> Not only is it not true in practice, it was clarified for C2x such that it's 
> definitely not true per standard, either (while, before, it was unclear and 
> could've been read either way). The current wording in C2x is: "The pointer 
> returned if the allocation succeeds is suitably aligned so that it may be 
> assigned to a pointer to any type of object with a fundamental alignment 
> requirement and size less than or equal to the size requested.", see 
> http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2293.htm

As the community's WG14 representative, I can confirm this information. We 
discussed N2293 in Pittsburgh in 2018 
(http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2375.pdf) and the proposal 
had unanimous consent from the committee. WG14 is still establishing their new 
DR process, so the committee had no way to flag this as a defect report that 
applies retroactively to prior standards, but given the purpose of the paper 
was to clarify the intent, I think it is reasonable for us to treat it as a 
defect in all C versions if we wish to do so, not just C2x.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118804

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

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In D118804#3302675 , @rsmith wrote:

> I support this revert.

Having received enough support, I'll go ahead and commit, and then propose 
backport to llvm 14 branch.

But --

> - `malloc` always returns storage whose alignment is at least the largest 
> fundamental alignment.

As has been discussed previously in this review thread, that's not true -- and 
the in-practice-falseness of this statement was the trigger for reverting the 
change.

Not only is it not true in practice, it was clarified for C2x such that it's 
definitely not true per standard, either (while, before, it was unclear and 
could've been read either way). The current wording in C2x is: "The pointer 
returned if the allocation succeeds is suitably aligned so that it may be 
assigned to a pointer to any type of object with a fundamental alignment 
requirement and size less than or equal to the size requested.", see 
http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2293.htm


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118804

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

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

I support this revert.

`malloc`, `operator new`, and `operator new[]` (by the latter two I mean the 
usual global allocation functions, not user-provided ones) follow these rules:

- `malloc` always returns storage whose alignment is at least the largest 
fundamental alignment. This allows, for example, `T *p = (T*)malloc(n * 
sizeof(T) + 1);` for any non-overaligned type `T`, even when `n` is zero. (The 
actual alignment might depend on the allocation size, but can never be less 
than `max(_Alignof(long double), _Alignof(long long))`, even if the allocation 
size is lower. We could easily add an alignment assumption stating that fact if 
we so chose.)
- `operator new(n)` always returns storage whose alignment is sufficient for 
any type `T` with `sizeof(T) == n && alignof(T) <= 
__STDCPP_DEFAULT_NEW_ALIGNMENT__`. For example, `operator new(n)` for any odd 
size `n` is always permitted to return 1-byte aligned storage. This makes sense 
because `operator new` is expected to power (only) `new T`.
- `operator new[](n)` always returns storage whose alignment is sufficient for 
any type `T` with `sizeof(T) <= n && alignof(T) <= 
__STDCPP_DEFAULT_NEW_ALIGNMENT__`. This is necessary to support things like 
`new std::byte[n]` followed by putting objects into that storage. However, 
unlike the `malloc` rules, this doesn't support zero-sized arrays in general.

Note that these are three different rules and we should not confuse them. The 
`malloc` rules have nothing to do with the default new alignment.

One possible and intended use of the C++ new alignment rules is to set the 
default new alignment to *less* than that guaranteed by `malloc`. For example, 
using `-fnew-alignment=1` can be used to pass alignment information into the 
allocator whenever possible, which may result in more efficient code 
generation. This flag and its effect on `operator new` are entirely unrelated 
to the behavior of `malloc`, and should not reduce the alignment that is 
assumed to be provided by `malloc`.

Providing information to LLVM about the expected alignment of memory returned 
by `malloc` -- if we have that information and LLVM does not -- seems like a 
good thing, but this is not the way to do it. Adding a separate, 
target-specific notion of malloc alignment, and using that as the default for 
the new alignment, might be reasonable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118804

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

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

Also, __builtin_malloc(...) can be used to avoid any alignment assumptions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118804

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

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

[Alex Lapenkov via Phabricator via cfe-commits]

Lapenkov added a comment.

In D118804#3301753 , @jyknight wrote:

> In D118804#3301746 , @Lapenkov 
> wrote:
>
>> Is it hard to retrofit this diff into LLVM 13?
>
> It would be easy to backport this change into LLVM 13, but the problem is 
> that there are no more releases planned on the LLVM 13 branch.

I see, thanks for the quick reply.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118804

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

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In D118804#3301746 , @Lapenkov wrote:

> Is it hard to retrofit this diff into LLVM 13?

It would be easy to backport this change into LLVM 13, but the problem is that 
there are no more releases planned on the LLVM 13 branch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118804

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

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

[Alex Lapenkov via Phabricator via cfe-commits]

Lapenkov added a comment.

Frankly, sounds quite inconvenient. This means that the default behavior 
changes for LLVM 13 and then changes back for LLVM 14.
Everyone using a custom allocator with weak alignment will need to know about 
this caveat and be really wary of this additional flag to ensure everything 
works as expected. This just burdens every user of 
Clang+(tcmalloc|jemalloc|mimalloc) with unnecessary steps.

Is it hard to retrofit this diff into LLVM 13?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118804

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

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

llvm 14 - yes

llvm 13.0.2 is not planned.

If you need a workaround, use -fno-builtin-malloc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118804

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

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

[Alex Lapenkov via Phabricator via cfe-commits]

Lapenkov added a comment.

Do you think it should be ported to release/13.x too?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118804

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

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

[Dávid Bolvanský via Phabricator via cfe-commits]

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

LG.

Please wait a day in case any new comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118804

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

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

[James Y Knight via Phabricator via cfe-commits]

jyknight updated this revision to Diff 406478.
jyknight added a comment.

Rebase, and preserve (and modify) test-case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118804

Files:
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CodeGen/alloc-fns-alignment.c

Index: clang/test/CodeGen/alloc-fns-alignment.c
===
--- clang/test/CodeGen/alloc-fns-alignment.c
+++ clang/test/CodeGen/alloc-fns-alignment.c
@@ -1,12 +1,9 @@
-// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm < %s | FileCheck %s --check-prefix=ALIGN16
-// RUN: %clang_cc1 -triple x86_64-windows-msvc  -emit-llvm < %s | FileCheck %s --check-prefix=ALIGN16
-// RUN: %clang_cc1 -triple i386-apple-darwin-emit-llvm < %s | FileCheck %s --check-prefix=ALIGN16
-// RUN: %clang_cc1 -triple i386-unknown-linux-gnu   -emit-llvm < %s | FileCheck %s --check-prefix=ALIGN8
-// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fno-builtin-malloc  -emit-llvm < %s  | FileCheck %s --check-prefix=NOBUILTIN-MALLOC
-// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fno-builtin-calloc  -emit-llvm < %s  | FileCheck %s --check-prefix=NOBUILTIN-CALLOC
-// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fno-builtin-realloc -emit-llvm < %s  | FileCheck %s --check-prefix=NOBUILTIN-REALLOC
-// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fno-builtin-aligned_alloc -emit-llvm < %s  | FileCheck %s --check-prefix=NOBUILTIN-ALIGNED_ALLOC
-// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fno-builtin-memalign -emit-llvm < %s  | FileCheck %s --check-prefix=NOBUILTIN-MEMALIGN
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm < %s | FileCheck %s
+
+// Note: this test originally asserted that malloc/calloc/realloc got alignment
+// attributes on their return pointer. However, that was reverted in
+// https://reviews.llvm.org/D118804 and it now asserts that they do _NOT_ get
+// align attributes.
 
 typedef __SIZE_TYPE__ size_t;
 
@@ -49,57 +46,36 @@
 }
 
 // CHECK-LABEL: @malloc_test
-// ALIGN16: align 16 i8* @malloc
-
-// CHECK-LABEL: @calloc_test
-// ALIGN16: align 16 i8* @calloc
-
-// CHECK-LABEL: @realloc_test
-// ALIGN16: align 16 i8* @realloc
+// CHECK: call i8* @malloc
 
-// CHECK-LABEL: @aligned_alloc_variable_test
-// ALIGN16:  %[[ALLOCATED:.*]] = call align 16 i8* @aligned_alloc({{i32|i64}} noundef %[[ALIGN:.*]], {{i32|i64}} noundef %[[NBYTES:.*]])
-// ALIGN16-NEXT: call void @llvm.assume(i1 true) [ "align"(i8* %[[ALLOCATED]], {{i32|i64}} %[[ALIGN]]) ]
-
-// CHECK-LABEL: @memalign_variable_test
-// ALIGN16:  %[[ALLOCATED:.*]] = call align 16 i8* @memalign({{i32|i64}} noundef %[[ALIGN:.*]], {{i32|i64}} noundef %[[NBYTES:.*]])
-// ALIGN16-NEXT: call void @llvm.assume(i1 true) [ "align"(i8* %[[ALLOCATED]], {{i32|i64}} %[[ALIGN]]) ]
-
-// CHECK-LABEL: @aligned_alloc_constant_test
-// ALIGN16: align 16 i8* @aligned_alloc
-
-// CHECK-LABEL: @aligned_alloc_large_constant_test
-// ALIGN16: align 4096 i8* @aligned_alloc
-
-// CHECK-LABEL: @memalign_large_constant_test
-// ALIGN16: align 4096 i8* @memalign
-
-// CHECK-LABEL: @malloc_test
-// ALIGN8: align 8 i8* @malloc
+// CHECK: declare i8* @malloc
 
 // CHECK-LABEL: @calloc_test
-// ALIGN8: align 8 i8* @calloc
+// CHECK: call i8* @calloc
+
+// CHECK: declare i8* @calloc
 
 // CHECK-LABEL: @realloc_test
-// ALIGN8: align 8 i8* @realloc
+// CHECK: call i8* @realloc
+
+// CHECK: declare i8* @realloc
 
 // CHECK-LABEL: @aligned_alloc_variable_test
-// ALIGN8: align 8 i8* @aligned_alloc
+// CHECK:  %[[ALLOCATED:.*]] = call i8* @aligned_alloc({{i32|i64}} noundef %[[ALIGN:.*]], {{i32|i64}} noundef %[[NBYTES:.*]])
+// CHECK-NEXT: call void @llvm.assume(i1 true) [ "align"(i8* %[[ALLOCATED]], {{i32|i64}} %[[ALIGN]]) ]
+
+// CHECK: declare i8* @aligned_alloc
 
 // CHECK-LABEL: @memalign_variable_test
-// ALIGN8: align 8 i8* @memalign
+// CHECK:  %[[ALLOCATED:.*]] = call i8* @memalign({{i32|i64}} noundef %[[ALIGN:.*]], {{i32|i64}} noundef %[[NBYTES:.*]])
+// CHECK-NEXT: call void @llvm.assume(i1 true) [ "align"(i8* %[[ALLOCATED]], {{i32|i64}} %[[ALIGN]]) ]
 
 // CHECK-LABEL: @aligned_alloc_constant_test
-// ALIGN8: align 8 i8* @aligned_alloc
+// CHECK: call align 8 i8* @aligned_alloc
 
 // CHECK-LABEL: @aligned_alloc_large_constant_test
-// ALIGN8: align 4096 i8* @aligned_alloc
+// CHECK: call align 4096 i8* @aligned_alloc
 
 // CHECK-LABEL: @memalign_large_constant_test
-// ALIGN8: align 4096 i8* @memalign
+// CHECK: align 4096 i8* @memalign
 
-// NOBUILTIN-MALLOC: declare i8* @malloc
-// NOBUILTIN-CALLOC: declare i8* @calloc
-// NOBUILTIN-REALLOC: declare i8* @realloc
-// NOBUILTIN-ALIGNED_ALLOC: declare i8* @aligned_alloc
-// NOBUILTIN-MEMALIGN: declare i8* @memalign
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 requested changes to this revision.
xbolva00 added inline comments.
This revision now requires changes to proceed.



Comment at: clang/test/CodeGen/alloc-fns-alignment.c:37
-
-void *aligned_alloc_large_constant_test(size_t n) {
-  return aligned_alloc(4096, n);

These tests for should stay.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118804

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

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

[Yuanfang Chen via Phabricator via cfe-commits]

ychen accepted this revision.
ychen added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118804

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

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

[Yuanfang Chen via Phabricator via cfe-commits]

ychen added a comment.

In D118804#3292263 , @jyknight wrote:

> In D118804#3292185 , @ychen wrote:
>
>> I don't see why the patch is wrong... It uses the target/platform-specific 
>> `NewAlign`. If the platform allows customized memory allocation that assumes 
>> weak alignment, it should set the `NewAlign` accordingly, no?
>
> NewAlign is set to the largest object alignment for which the compiler can 
> call `::operator new(size_t)`, instead of calling `::operator new(size_t, 
> align_t)`, so, no, you definitely wouldn't want to change that value.
>
> That is: it is correctly set to 16 if `::operator new(16)`` will return 
> 16-byte-aligned memory, even if `::operator new(8)` will return an 
> 8-byte-aligned memory. That's because 8-byte-aligned memory is 
> "suitably-aligned" for any possible object that can fit in 8 bytes.

If I understand it correctly, this is assuming a malloc that could return a 
different alignment depending on the allocation size. Some other malloc always 
returns the same alignment (which the reverted patch has assumed). Yeah, then 
the reverted patch needs to do it in a platform-specific way (default opt-out?).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118804

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

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In D118804#3292185 , @ychen wrote:

> I don't see why the patch is wrong... It uses the target/platform-specific 
> `NewAlign`. If the platform allows customized memory allocation that assumes 
> weak alignment, it should set the `NewAlign` accordingly, no?

NewAlign is set to the largest object alignment for which the compiler can call 
`::operator new(size_t)`, instead of calling `::operator new(size_t, align_t)`, 
so, no, you definitely wouldn't want to change that value.

That is: it is correctly set to 16 if `::operator new(16)`` will return 
16-byte-aligned memory, even if `::operator new(8)` will return an 
8-byte-aligned memory. That's because 8-byte-aligned memory is 
"suitably-aligned" for any possible object that can fit in 8 bytes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118804

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

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

In D118804#3292179 , @MaskRay wrote:

> In D118804#3292176 , @xbolva00 
> wrote:
>
 Reintroducing an optimization like this with an additional check that the 
 allocation size is large enough should be valid everywhere.
>>
>> Should not be hard, could you do it? Then LGTM.
>
> @xbolva00 While I appreciate you contribution to the optimizations, I am not 
> sure the burden of fixing a broken optimization lays on the issue 
> reporter(s)... (quite a few now).
> The revert is the safest choice both for origin/main and the now created 
> origin/release/14.x.
> If you are still motivated to improve the situation, you can contribute after 
> the original broken patch is reverted...

I still think we can spend some more minutes to discuss it properly than 
rushing with revert.

Mozilla folks reported after 7-8 months in trunk? So we can spend day or two to 
discuss it properly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118804

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

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

Can you also confirm that there is no similar related to op new?

Yes, we “can” this assumption by removing it but.. there should be consensus 
whether just this code is problematic or problem is bigger (as we use 
NewAlign..)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118804

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

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

[Yuanfang Chen via Phabricator via cfe-commits]

ychen added a comment.

I don't see why the patch is wrong... It uses the target/platform-specific 
`NewAlign`. If the platform allows customized memory allocation that assumes 
weak alignment, it should set the `NewAlign` accordingly, no?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118804

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

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D118804#3292176 , @xbolva00 wrote:

>>> Reintroducing an optimization like this with an additional check that the 
>>> allocation size is large enough should be valid everywhere.
>
> Should not be hard, could you do it? Then LGTM.

@xbolva00 While I appreciate you contribution to the optimizations, I am not 
sure the burden of fixing a broken optimization lays on the issue reporter...
The revert is the safest choice both for origin/main and the now created 
origin/release/14.x.
If you are still motivated to improve the situation, you can contribute after 
the original broken patch is reverted...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118804

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

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

>> Reintroducing an optimization like this with an additional check that the 
>> allocation size is large enough should be valid everywhere.

Should not be hard, could you do it? Then LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118804

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

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

> For C++, I confess I have some problems interpreting this sentence:
>
> (https://eel.is/c++draft/cpp.predefined)
>
>> `__STDCPP_­DEFAULT_­NEW_­ALIGNMENT__`:  An integer literal of type 
>> std::size_t whose value is the alignment guaranteed by a call to operator 
>> new(std::size_t) or operator new[](std::size_t). [ Note: Larger alignments 
>> will be passed to operator new(std::size_t, std::align_val_t), etc. (8.3.4). 
>> — end note ]
>
> It seems to suggest that a large alignment may be assumed, but practically, I 
> know at least mimalloc uses 8-byte alignment for 64-bit Darwin, not 16-byte 
> alignment.

See https://eel.is/c++draft/basic.stc.dynamic.allocation#3 (this text was moved 
around a bit since C++17, but it hasn't changed meaning). `operator 
new(std::size_t)` requires that "the storage is aligned for any object that 
does not have new-extended alignment and is of the requested size." ()That is, 
if you have a size 8 allocation, it is not required to be 16-byte aligned.

> [...] This needs to be very careful since 
> `__STDCPP_­DEFAULT_­NEW_­ALIGNMENT__` doesn't seem to match the practice.

I don't think that's true? I believe that macro is generally being set / 
handled correctly. Reintroducing an optimization like this with an additional 
check that the allocation size is large enough should be valid everywhere.

> mimalloc uses 8-byte alignment for 64-bit Darwin, not 16-byte alignment.

It uses 8-byte alignment for 8-byte allocations, you mean. It uses 16-byte 
alignment for 16-byte (or larger) allocations (doing otherwise would be a clear 
bug, given that max_align_t is 16).

In D118804#3291060 , @xbolva00 wrote:

> Or can we bailout just for gnu? and preserve this for darwin?

This isn't a GNU-only issue, so I don't think that makes sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118804

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

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

>> mimalloc uses 8-byte alignment for 64-bit Darwin, not 16-byte alignment.

Than they are doing something fishy if I understood rjmccall’s comment 
correctly about darwin.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118804

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

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D118804#3291060 , @xbolva00 wrote:

> Or can we bailout just for gnu? and preserve this for darwin?

As is, the original patch applies the C++ getNewAlign to C 
calloc/malloc/strdup/etc. This is outright wrong.

For C++, I confess I have some problems interpreting this sentence:

(https://eel.is/c++draft/cpp.predefined)

> `__STDCPP_­DEFAULT_­NEW_­ALIGNMENT__`:  An integer literal of type 
> std::size_t whose value is the alignment guaranteed by a call to operator 
> new(std::size_t) or operator new[](std::size_t). [ Note: Larger alignments 
> will be passed to operator new(std::size_t, std::align_val_t), etc. (8.3.4). 
> — end note ]

It seems to suggest that a large alignment may be assumed, but practically, I 
know at least mimalloc uses 8-byte alignment for 64-bit Darwin, not 16-byte 
alignment.

> Or can we bailout just for gnu? and preserve this for darwin?

If deemed useful, someone may create a subsequent patch applying a 
fixed/restricted form.
This needs to be very careful since `__STDCPP_­DEFAULT_­NEW_­ALIGNMENT__` 
doesn't seem to match the practice.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118804

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

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

Or can we bailout just for gnu? and preserve this for darwin?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118804

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

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

[James Y Knight via Phabricator via cfe-commits]

jyknight added a subscriber: collares.
jyknight added a comment.

In D118804#3290874 , @collares wrote:

> might be worth filing a GCC bug as well

Yes, a bug report for GCC should be opened as well. @collares do you want to 
take that?

In D118804#3290937 , @xbolva00 wrote:

> So better to find solution / agreement with gcc devs too.

Reverting to the previous behavior is safe for clang to do in any case, and 
thus I think we ought to. Subsequently landing a change that restricts the 
alignment assumptions based on allocation size would be fine, as well. But 
reverting the incorrect change doesn't need to wait for that to be written, and 
neither need wait for a discussion with GCC.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118804

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

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

[Fangrui Song via Phabricator via cfe-commits]

MaskRay accepted this revision.
MaskRay added a comment.

LGTM. This should be ported to release/14.x as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118804

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

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added subscribers: urnathan, MaskRay.
MaskRay added a comment.

I agree that the original patch (c2297544c04764237cedc523083c7be2fb3833d4 
) should 
be reverted.

The `__STDCPP_DEFAULT_NEW_ALIGNMENT__==16` (on x86-64) thing is the glibc 
behavior.
It should be taken with a grain of salt since many malloc implementations don't 
provide the guarantee for allocations with smaller size 
(jemalloc/mimalloc/tcmalloc just guarantee 8 alignment on 64-bit systems).
And of course, this doesn't apply to to malloc and friends.

@urnathan mentioned this on https://github.com/llvm/llvm-project/issues/53540

> Notice the 'suitably-sized allocation' caveat. Either poor wording, or 
> getNewAlign is not guaranteeing smaller alignment for smaller allocations. I 
> have not audited all the uses of getNewAlign to see if they presume the value 
> is a conditional guarantee or not.



> GCC also does same assumptions

If GCC makes use of  `__STDCPP_DEFAULT_NEW_ALIGNMENT__==16` for malloc or 
operator new with smaller allocations, looks like it is a bug that should be 
fixed as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118804

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

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In D118804#3290874 , @collares wrote:

> It is worth noting that GCC started assuming 16-byte alignment for small 
> objects in 2016, before N2293 was written and accepted into C2x; see 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90569#c8 and 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90569#c9 for more recent 
> (informal) statements from GCC developers.

Note that linked bug is about assumed alignment (to decide which allocator to 
call for C++ "new X", which uses max_align_t as part of its decision (quite 
reasonably). For malloc assumed-alignment, GCC uses the value 
MALLOC_ABI_ALIGNMENT (e.g. 
https://github.com/gcc-mirror/gcc/blob/c123096cf14ac87875bba51279e46cceeb18faa1/gcc/tree-ssa-ccp.cc#L2336),
 which is set to BITS_PER_WORD by default, unless overridden by a target 
config. On x86, it's not overridden, and thus is BITS_PER_WORD (so 32 or 64)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118804

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

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

So better to find solution / agreement with gcc devs too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118804

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

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In D118804#3290803 , @xbolva00 wrote:

> GCC also does same assumptions

Looking at GCC: GCC only assumes 4-byte alignment on i386, and 8-byte alignment 
on x86-64, which is why it hasn't actively broken users, unlike the clang 
change.

However, I think that's also still a bug in GCC. It should not be assuming 
8-byte alignment for a 1-byte allocation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118804

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

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

[James Y Knight via Phabricator via cfe-commits]

jyknight updated this revision to Diff 405315.
jyknight added a comment.

(fix git mishap: neglected to add a file to original change after conflict 
resolution)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118804

Files:
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CodeGen/alloc-fns-alignment.c

Index: clang/test/CodeGen/alloc-fns-alignment.c
===
--- clang/test/CodeGen/alloc-fns-alignment.c
+++ /dev/null
@@ -1,81 +0,0 @@
-// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm < %s | FileCheck %s --check-prefix=ALIGN16
-// RUN: %clang_cc1 -triple x86_64-windows-msvc  -emit-llvm < %s | FileCheck %s --check-prefix=ALIGN16
-// RUN: %clang_cc1 -triple i386-apple-darwin-emit-llvm < %s | FileCheck %s --check-prefix=ALIGN16
-// RUN: %clang_cc1 -triple i386-unknown-linux-gnu   -emit-llvm < %s | FileCheck %s --check-prefix=ALIGN8
-// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fno-builtin-malloc  -emit-llvm < %s  | FileCheck %s --check-prefix=NOBUILTIN-MALLOC
-// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fno-builtin-calloc  -emit-llvm < %s  | FileCheck %s --check-prefix=NOBUILTIN-CALLOC
-// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fno-builtin-realloc -emit-llvm < %s  | FileCheck %s --check-prefix=NOBUILTIN-REALLOC
-// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fno-builtin-aligned_alloc -emit-llvm < %s  | FileCheck %s --check-prefix=NOBUILTIN-ALIGNED_ALLOC
-
-typedef __SIZE_TYPE__ size_t;
-
-void *malloc(size_t);
-void *calloc(size_t, size_t);
-void *realloc(void *, size_t);
-void *aligned_alloc(size_t, size_t);
-
-void *malloc_test(size_t n) {
-  return malloc(n);
-}
-
-void *calloc_test(size_t n) {
-  return calloc(1, n);
-}
-
-void *realloc_test(void *p, size_t n) {
-  return realloc(p, n);
-}
-
-void *aligned_alloc_variable_test(size_t n, size_t a) {
-  return aligned_alloc(a, n);
-}
-
-void *aligned_alloc_constant_test(size_t n) {
-  return aligned_alloc(8, n);
-}
-
-void *aligned_alloc_large_constant_test(size_t n) {
-  return aligned_alloc(4096, n);
-}
-
-// CHECK-LABEL: @malloc_test
-// ALIGN16: align 16 i8* @malloc
-
-// CHECK-LABEL: @calloc_test
-// ALIGN16: align 16 i8* @calloc
-
-// CHECK-LABEL: @realloc_test
-// ALIGN16: align 16 i8* @realloc
-
-// CHECK-LABEL: @aligned_alloc_variable_test
-// ALIGN16:  %[[ALLOCATED:.*]] = call align 16 i8* @aligned_alloc({{i32|i64}} noundef %[[ALIGN:.*]], {{i32|i64}} noundef %[[NBYTES:.*]])
-// ALIGN16-NEXT: call void @llvm.assume(i1 true) [ "align"(i8* %[[ALLOCATED]], {{i32|i64}} %[[ALIGN]]) ]
-
-// CHECK-LABEL: @aligned_alloc_constant_test
-// ALIGN16: align 16 i8* @aligned_alloc
-
-// CHECK-LABEL: @aligned_alloc_large_constant_test
-// ALIGN16: align 4096 i8* @aligned_alloc
-
-// CHECK-LABEL: @malloc_test
-// ALIGN8: align 8 i8* @malloc
-
-// CHECK-LABEL: @calloc_test
-// ALIGN8: align 8 i8* @calloc
-
-// CHECK-LABEL: @realloc_test
-// ALIGN8: align 8 i8* @realloc
-
-// CHECK-LABEL: @aligned_alloc_variable_test
-// ALIGN8: align 8 i8* @aligned_alloc
-
-// CHECK-LABEL: @aligned_alloc_constant_test
-// ALIGN8: align 8 i8* @aligned_alloc
-
-// CHECK-LABEL: @aligned_alloc_large_constant_test
-// ALIGN8: align 4096 i8* @aligned_alloc
-
-// NOBUILTIN-MALLOC: declare i8* @malloc
-// NOBUILTIN-CALLOC: declare i8* @calloc
-// NOBUILTIN-REALLOC: declare i8* @realloc
-// NOBUILTIN-ALIGNED_ALLOC: declare i8* @aligned_alloc
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -15319,24 +15319,6 @@
   if (!FD->hasAttr())
 FD->addAttr(AllocAlignAttr::CreateImplicit(Context, ParamIdx(1, FD),
FD->getLocation()));
-  LLVM_FALLTHROUGH;
-case Builtin::BIcalloc:
-case Builtin::BImalloc:
-case Builtin::BImemalign:
-case Builtin::BIrealloc:
-case Builtin::BIstrdup:
-case Builtin::BIstrndup: {
-  if (!FD->hasAttr()) {
-unsigned NewAlign = Context.getTargetInfo().getNewAlign() /
-Context.getTargetInfo().getCharWidth();
-IntegerLiteral *Alignment = IntegerLiteral::Create(
-Context, Context.MakeIntValue(NewAlign, Context.UnsignedIntTy),
-Context.UnsignedIntTy, FD->getLocation());
-FD->addAttr(AssumeAlignedAttr::CreateImplicit(
-Context, Alignment, /*Offset=*/nullptr, FD->getLocation()));
-  }
-  break;
-}
 default:
   break;
 }
Index: clang/include/clang/Basic/TargetInfo.h
===
--- clang/include/clang/Basic/TargetInfo.h
+++ clang/include/clang/Basic/TargetInfo.h
@@ -646,8 +646,8 @@
   }
 
   /// Return the largest alignment for which a suitably-sized allocation with
-  

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

[Mauricio Collares via Phabricator via cfe-commits]

collares added a comment.

It is worth noting that GCC started assuming 16-byte alignment for small 
objects in 2016, before N2293 was written and accepted into C2x; see 
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90569#c8 and 
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90569#c9 for more recent 
(informal) statements from GCC developers. Other malloc implementations, such 
as tcmalloc (see https://google.github.io/tcmalloc/reference.html) also seem to 
be planning on relaxing the alignment assumptions once C2x compliance is 
widespread. It might be worth filing a GCC bug as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118804

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

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 requested changes to this revision.
xbolva00 added a comment.
This revision now requires changes to proceed.

GCC also does same assumptions


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118804

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

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

[James Y Knight via Phabricator via cfe-commits]

jyknight created this revision.
jyknight added reviewers: aaron.ballman, rjmccall, jdoerfert, xbolva00.
jyknight requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

The above change assumed that malloc (and friends) would always
allocate memory to getNewAlign(), even for allocations which have a
smaller size. This is not actually required by spec (a 1-byte
allocation may validly have 1-byte alignment).

Some real-world malloc implementations do not provide this guarantee,
and thus this optimization is breaking programs.

Fixes #53540

This reverts commit c2297544c04764237cedc523083c7be2fb3833d4 
.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D118804

Files:
  clang/include/clang/Basic/TargetInfo.h
  clang/test/CodeGen/alloc-fns-alignment.c

Index: clang/test/CodeGen/alloc-fns-alignment.c
===
--- clang/test/CodeGen/alloc-fns-alignment.c
+++ /dev/null
@@ -1,81 +0,0 @@
-// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm < %s | FileCheck %s --check-prefix=ALIGN16
-// RUN: %clang_cc1 -triple x86_64-windows-msvc  -emit-llvm < %s | FileCheck %s --check-prefix=ALIGN16
-// RUN: %clang_cc1 -triple i386-apple-darwin-emit-llvm < %s | FileCheck %s --check-prefix=ALIGN16
-// RUN: %clang_cc1 -triple i386-unknown-linux-gnu   -emit-llvm < %s | FileCheck %s --check-prefix=ALIGN8
-// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fno-builtin-malloc  -emit-llvm < %s  | FileCheck %s --check-prefix=NOBUILTIN-MALLOC
-// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fno-builtin-calloc  -emit-llvm < %s  | FileCheck %s --check-prefix=NOBUILTIN-CALLOC
-// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fno-builtin-realloc -emit-llvm < %s  | FileCheck %s --check-prefix=NOBUILTIN-REALLOC
-// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fno-builtin-aligned_alloc -emit-llvm < %s  | FileCheck %s --check-prefix=NOBUILTIN-ALIGNED_ALLOC
-
-typedef __SIZE_TYPE__ size_t;
-
-void *malloc(size_t);
-void *calloc(size_t, size_t);
-void *realloc(void *, size_t);
-void *aligned_alloc(size_t, size_t);
-
-void *malloc_test(size_t n) {
-  return malloc(n);
-}
-
-void *calloc_test(size_t n) {
-  return calloc(1, n);
-}
-
-void *realloc_test(void *p, size_t n) {
-  return realloc(p, n);
-}
-
-void *aligned_alloc_variable_test(size_t n, size_t a) {
-  return aligned_alloc(a, n);
-}
-
-void *aligned_alloc_constant_test(size_t n) {
-  return aligned_alloc(8, n);
-}
-
-void *aligned_alloc_large_constant_test(size_t n) {
-  return aligned_alloc(4096, n);
-}
-
-// CHECK-LABEL: @malloc_test
-// ALIGN16: align 16 i8* @malloc
-
-// CHECK-LABEL: @calloc_test
-// ALIGN16: align 16 i8* @calloc
-
-// CHECK-LABEL: @realloc_test
-// ALIGN16: align 16 i8* @realloc
-
-// CHECK-LABEL: @aligned_alloc_variable_test
-// ALIGN16:  %[[ALLOCATED:.*]] = call align 16 i8* @aligned_alloc({{i32|i64}} noundef %[[ALIGN:.*]], {{i32|i64}} noundef %[[NBYTES:.*]])
-// ALIGN16-NEXT: call void @llvm.assume(i1 true) [ "align"(i8* %[[ALLOCATED]], {{i32|i64}} %[[ALIGN]]) ]
-
-// CHECK-LABEL: @aligned_alloc_constant_test
-// ALIGN16: align 16 i8* @aligned_alloc
-
-// CHECK-LABEL: @aligned_alloc_large_constant_test
-// ALIGN16: align 4096 i8* @aligned_alloc
-
-// CHECK-LABEL: @malloc_test
-// ALIGN8: align 8 i8* @malloc
-
-// CHECK-LABEL: @calloc_test
-// ALIGN8: align 8 i8* @calloc
-
-// CHECK-LABEL: @realloc_test
-// ALIGN8: align 8 i8* @realloc
-
-// CHECK-LABEL: @aligned_alloc_variable_test
-// ALIGN8: align 8 i8* @aligned_alloc
-
-// CHECK-LABEL: @aligned_alloc_constant_test
-// ALIGN8: align 8 i8* @aligned_alloc
-
-// CHECK-LABEL: @aligned_alloc_large_constant_test
-// ALIGN8: align 4096 i8* @aligned_alloc
-
-// NOBUILTIN-MALLOC: declare i8* @malloc
-// NOBUILTIN-CALLOC: declare i8* @calloc
-// NOBUILTIN-REALLOC: declare i8* @realloc
-// NOBUILTIN-ALIGNED_ALLOC: declare i8* @aligned_alloc
Index: clang/include/clang/Basic/TargetInfo.h
===
--- clang/include/clang/Basic/TargetInfo.h
+++ clang/include/clang/Basic/TargetInfo.h
@@ -646,8 +646,8 @@
   }
 
   /// Return the largest alignment for which a suitably-sized allocation with
-  /// '::operator new(size_t)' or 'malloc' is guaranteed to produce a
-  /// correctly-aligned pointer.
+  /// '::operator new(size_t)' is guaranteed to produce a correctly-aligned
+  /// pointer.
   unsigned getNewAlign() const {
 return NewAlign ? NewAlign : std::max(LongDoubleAlign, LongLongAlign);
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits