llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.26k stars 11.67k forks source link

no lifetime markers emitted for returned aggregates #68747

Open nickdesaulniers opened 11 months ago

nickdesaulniers commented 11 months ago

forked from #43598 (and https://reviews.llvm.org/D74094) which is C++ specific; there are different rules for C FWICT.

struct foo {
    long long x;
};

struct foo foo_factory (void);
struct foo div (struct foo, struct foo);

long long bar (void) {
    return div(div(foo_factory(), foo_factory()), div(foo_factory(), foo_factory())).x;
}

We should emit lifetime intrinsics for those temporary aggregates returned from functions

The latest C23 draft § 6.2.4 Storage durations of objects

8 A non-lvalue expression with structure or union type, where the structure or union contains a member with array type (including, recursively, members of all contained structures and unions) refers to an object with automatic storage duration and temporary lifetime. 39) Its lifetime begins when the expression is evaluated and its initial value is the value of the expression. Its lifetime ends when the evaluation of the containing full expression ends. Any attempt to modify an object with temporary lifetime results in undefined behavior. An object with temporary lifetime behaves as if it were declared with the type of its value for the purposes of effective type. Such an object need not have a unique address.

Speaking more with @AaronBallman , Aaron found https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1285.htm introduced the current wording into c11. https://wiki.sei.cmu.edu/confluence/display/c/EXP35-C.+Do+not+modify+objects+with+temporary+lifetime talks more about this. https://web.archive.org/web/20231010182056/https://yarchive.net/comp/struct_return.html talks more about this in regards to arrays within the returned struct.

Also, this is quite painful for 32b ARM (try building the above with -O2 -mabi=aapcs-linux --target=arm-linux-gnueabi) due to ARM AAPCS32 § 6.4 Result Return

A Composite Type larger than 4 bytes, or whose size cannot be determined statically by both caller and callee, is stored in memory at an address passed as an extra argument when the function was called (Parameter Passing (base PCS), Rule A.4). The memory to be used for the result may be modified at any point during the function call.

(I'll tag the ARM backend folks for the CC, though this isn't a backend specific bug) For other targets, clang unwraps the single i64 which results in much better codegen. Seems unfortunate the AAPCS32 didn't limit that rule about "composite types" to types larger than 8B rather than 4B.

So I think if the type contains an array, we need to have the lifetime be emitted after the full expression ends. CodeGenFunction::pushFullExprCleanup and CodeGenFunction::pushCleanupAfterFullExpr look useful, though I'm not sure that's precisely what they do (or what the difference is between them yet, maybe the latter is what I need). Otherwise I guess these temporaries can just be cleaned up immediately after the function call they are passed to.

cc @rjmccall @ilovepi

llvmbot commented 11 months ago

@llvm/issue-subscribers-clang-codegen

Author: Nick Desaulniers (nickdesaulniers)

forked from #43598 (and https://reviews.llvm.org/D74094) which is C++ specific; there are different rules for C FWICT. ```c struct foo { long long x; }; struct foo foo_factory (void); struct foo div (struct foo, struct foo); long long bar (void) { return div(div(foo_factory(), foo_factory()), div(foo_factory(), foo_factory())).x; } ``` We should emit lifetime intrinsics for those temporary aggregates returned from functions The [latest C23 draft](https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3096.pdf) § 6.2.4 Storage durations of objects > 8 A non-lvalue expression with structure or union type, where the structure or union contains a member with array type (including, recursively, members of all contained structures and unions) refers to an object with automatic storage duration and temporary lifetime. 39) Its lifetime begins when the expression is evaluated and its initial value is the value of the expression. Its lifetime ends when the evaluation of the containing full expression ends. Any attempt to modify an object with temporary lifetime results in undefined behavior. An object with temporary lifetime behaves as if it were declared with the type of its value for the purposes of effective type. Such an object need not have a unique address. Speaking more with @AaronBallman , Aaron found https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1285.htm introduced the current wording into c11. https://wiki.sei.cmu.edu/confluence/display/c/EXP35-C.+Do+not+modify+objects+with+temporary+lifetime talks more about this. https://web.archive.org/web/20231010182056/https://yarchive.net/comp/struct_return.html talks more about this in regards to arrays within the returned struct. Also, this is quite painful for 32b ARM due to [ARM AAPCS32](https://github.com/ARM-software/abi-aa/blob/main/aapcs32/aapcs32.rst#result-return) § 6.4 Result Return > A Composite Type larger than 4 bytes, or whose size cannot be determined statically by both caller and callee, is stored in memory at an address passed as an extra argument when the function was called ([Parameter Passing (base PCS)](https://github.com/ARM-software/abi-aa/blob/main/aapcs32/aapcs32.rst#parameter-passing-base-pcs), [Rule A.4](https://github.com/ARM-software/abi-aa/blob/main/aapcs32/aapcs32.rst#rule-a-4)). The memory to be used for the result may be modified at any point during the function call. (I'll tag the ARM backend folks for the CC, though this isn't a backend specific bug) So I think if the type contains an array, we need to have the lifetime be emitted after the full expression ends. `CodeGenFunction::pushFullExprCleanup` and `CodeGenFunction::pushCleanupAfterFullExpr` look useful, though I'm not sure that's precisely what they do (or what the difference is between them yet). Otherwise I guess these temporaries can just be cleaned up after the function call they are passed to.
llvmbot commented 11 months ago

@llvm/issue-subscribers-c

Author: Nick Desaulniers (nickdesaulniers)

forked from #43598 (and https://reviews.llvm.org/D74094) which is C++ specific; there are different rules for C FWICT. ```c struct foo { long long x; }; struct foo foo_factory (void); struct foo div (struct foo, struct foo); long long bar (void) { return div(div(foo_factory(), foo_factory()), div(foo_factory(), foo_factory())).x; } ``` We should emit lifetime intrinsics for those temporary aggregates returned from functions The [latest C23 draft](https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3096.pdf) § 6.2.4 Storage durations of objects > 8 A non-lvalue expression with structure or union type, where the structure or union contains a member with array type (including, recursively, members of all contained structures and unions) refers to an object with automatic storage duration and temporary lifetime. 39) Its lifetime begins when the expression is evaluated and its initial value is the value of the expression. Its lifetime ends when the evaluation of the containing full expression ends. Any attempt to modify an object with temporary lifetime results in undefined behavior. An object with temporary lifetime behaves as if it were declared with the type of its value for the purposes of effective type. Such an object need not have a unique address. Speaking more with @AaronBallman , Aaron found https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1285.htm introduced the current wording into c11. https://wiki.sei.cmu.edu/confluence/display/c/EXP35-C.+Do+not+modify+objects+with+temporary+lifetime talks more about this. https://web.archive.org/web/20231010182056/https://yarchive.net/comp/struct_return.html talks more about this in regards to arrays within the returned struct. Also, this is quite painful for 32b ARM due to [ARM AAPCS32](https://github.com/ARM-software/abi-aa/blob/main/aapcs32/aapcs32.rst#result-return) § 6.4 Result Return > A Composite Type larger than 4 bytes, or whose size cannot be determined statically by both caller and callee, is stored in memory at an address passed as an extra argument when the function was called ([Parameter Passing (base PCS)](https://github.com/ARM-software/abi-aa/blob/main/aapcs32/aapcs32.rst#parameter-passing-base-pcs), [Rule A.4](https://github.com/ARM-software/abi-aa/blob/main/aapcs32/aapcs32.rst#rule-a-4)). The memory to be used for the result may be modified at any point during the function call. (I'll tag the ARM backend folks for the CC, though this isn't a backend specific bug) So I think if the type contains an array, we need to have the lifetime be emitted after the full expression ends. `CodeGenFunction::pushFullExprCleanup` and `CodeGenFunction::pushCleanupAfterFullExpr` look useful, though I'm not sure that's precisely what they do (or what the difference is between them yet). Otherwise I guess these temporaries can just be cleaned up after the function call they are passed to.
llvmbot commented 11 months ago

@llvm/issue-subscribers-backend-arm

Author: Nick Desaulniers (nickdesaulniers)

forked from #43598 (and https://reviews.llvm.org/D74094) which is C++ specific; there are different rules for C FWICT. ```c struct foo { long long x; }; struct foo foo_factory (void); struct foo div (struct foo, struct foo); long long bar (void) { return div(div(foo_factory(), foo_factory()), div(foo_factory(), foo_factory())).x; } ``` We should emit lifetime intrinsics for those temporary aggregates returned from functions The [latest C23 draft](https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3096.pdf) § 6.2.4 Storage durations of objects > 8 A non-lvalue expression with structure or union type, where the structure or union contains a member with array type (including, recursively, members of all contained structures and unions) refers to an object with automatic storage duration and temporary lifetime. 39) Its lifetime begins when the expression is evaluated and its initial value is the value of the expression. Its lifetime ends when the evaluation of the containing full expression ends. Any attempt to modify an object with temporary lifetime results in undefined behavior. An object with temporary lifetime behaves as if it were declared with the type of its value for the purposes of effective type. Such an object need not have a unique address. Speaking more with @AaronBallman , Aaron found https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1285.htm introduced the current wording into c11. https://wiki.sei.cmu.edu/confluence/display/c/EXP35-C.+Do+not+modify+objects+with+temporary+lifetime talks more about this. https://web.archive.org/web/20231010182056/https://yarchive.net/comp/struct_return.html talks more about this in regards to arrays within the returned struct. Also, this is quite painful for 32b ARM due to [ARM AAPCS32](https://github.com/ARM-software/abi-aa/blob/main/aapcs32/aapcs32.rst#result-return) § 6.4 Result Return > A Composite Type larger than 4 bytes, or whose size cannot be determined statically by both caller and callee, is stored in memory at an address passed as an extra argument when the function was called ([Parameter Passing (base PCS)](https://github.com/ARM-software/abi-aa/blob/main/aapcs32/aapcs32.rst#parameter-passing-base-pcs), [Rule A.4](https://github.com/ARM-software/abi-aa/blob/main/aapcs32/aapcs32.rst#rule-a-4)). The memory to be used for the result may be modified at any point during the function call. (I'll tag the ARM backend folks for the CC, though this isn't a backend specific bug) So I think if the type contains an array, we need to have the lifetime be emitted after the full expression ends. `CodeGenFunction::pushFullExprCleanup` and `CodeGenFunction::pushCleanupAfterFullExpr` look useful, though I'm not sure that's precisely what they do (or what the difference is between them yet). Otherwise I guess these temporaries can just be cleaned up after the function call they are passed to.
nickdesaulniers commented 10 months ago

pushCleanupAfterFullExpr<CallLifetimeEnd> looks like it might be reusable here. (or pushFullExprCleanup or CallLifetimeEnd)