For a default visibility external linkage definition, dso_local is set for ELF
-fno-pic/-fpie and COFF and Mach-O. Since default clang -cc1 for ELF is similar
to -fpic ("PIC Level" is not set), this nuance causes unneeded binary format differences.
To make emitted IR similar, ELF -cc1 -fpic will default to -fno-semantic-interposition,
which sets dso_local for default visibility external linkage definitions.
To make this flip smooth and enable future (dso_local as definition default),
this patch replaces (function) `define ` with `define{{.*}} `,
(variable/constant/alias) `= ` with `={{.*}} `, or inserts appropriate `{{.*}} `.
arguments.
* Adds 'nonnull' and 'dereferenceable(N)' to 'this' pointer arguments
* Gates 'nonnull' on -f(no-)delete-null-pointer-checks
* Introduces this-nonnull.cpp and microsoft-abi-this-nullable.cpp tests to
explicitly test the behavior of this change
* Refactors hundreds of over-constrained clang tests to permit these
attributes, where needed
* Updates Clang12 patch notes mentioning this change
Reviewed-by: rsmith, jdoerfert
Differential Revision: https://reviews.llvm.org/D17993
The original bug was discovered in T75057860. Clang front-end emits an AST that looks like this for an co_await expression:
|- ExprWithCleanups
|- -CoawaitExpr
|- -MaterializeTemporaryExpr ... Awaiter
...
|- -CXXMemberCallExpr ... .await_ready
...
|- -CallExpr ... __builtin_coro_resume
...
|- -CXXMemberCallExpr ... .await_resume
...
ExprWithCleanups is responsible for cleaning up (including calling dtors) for the temporaries generated in the wrapping expression).
In the above structure, the __builtin_coro_resume part (which corresponds to the code for the suspend case in the co_await with symmetric transfer), the pseudocode looks like this:
__builtin_coro_resume(
awaiter.await_suspend(
from_address(
__builtin_coro_frame())).address());
One of the temporaries that's generated as part of this code is the coroutine handle returned from awaiter.await_suspend() call. The call returns a handle which is a prvalue (since it's a returned value on the fly). In order to call the address() method on it, it needs to be converted into an xvalue. Hence a materialized temp is created to hold it. This temp will need to be cleaned up eventually. Now, since all cleanups happen at the end of the entire co_await expression, which is after the <coro.suspend> suspension point, the compiler will think that such a temp needs to live across suspensions, and need to be put on the coroutine frame, even though it's only used temporarily just to call address() method.
Such a phenomena not only unnecessarily increases the frame size, but can lead to ASAN failures, if the coroutine was already destroyed as part of the await_suspend() call. This is because if the coroutine was already destroyed, the frame no longer exists, and one can not store anything into it. But if the temporary object is considered to need to live on the frame, it will be stored into the frame after await_suspend() returns.
A fix attempt was done in https://reviews.llvm.org/D87470. Unfortunately it is incorrect. The reason is that cleanups in Clang works more like linearly than nested. There is one current state indicating whether it needs cleanup, and an ExprWithCleanups resets that state. This means that an ExprWithCleanups must be capable of cleaning up all temporaries created in the wrapping expression, otherwise there will be dangling temporaries cleaned up at the wrong place.
I eventually found a walk-around (https://reviews.llvm.org/D89066) that doesn't break any existing tests while fixing the issue. But it targets the final co_await only. If we ever have a co_await that's not on the final awaiter and the frame gets destroyed after suspend, we are in trouble. Hence we need a proper fix.
This patch is the proper fix. It does the folllowing things to fully resolve the issue:
1. The AST has to be generated in the order according to their nesting relationship. We should not generate AST out of order because then the code generator would incorrectly track the state of temporaries and when a cleanup is needed. So the code in buildCoawaitCalls is reorganized so that we will be generating the AST for each coawait member call in order along with their child AST.
2. await_ready() call is wrapped with an ExprWithCleanups so that temporaries in it gets cleaned up as early as possible to avoid living across suspension.
3. await_suspend() call is wrapped with an ExprWithCleanups if it's not a symmetric transfer. In the case of a symmetric transfer, in order to maintain the musttail call contract, the ExprWithCleanups is wraaped before the resume call.
4. In the end, we mark again that it needs a cleanup, so that the entire CoawaitExpr will be wrapped with a ExprWithCleanups which will clean up the Awaiter object associated with the await expression.
Differential Revision: https://reviews.llvm.org/D90990
Some tests start to fail after https://reviews.llvm.org/D89066.
It's because the size of pointers are different on different targets.
Limit the target in the command so there is no confusion.
Also noticed I had typo in the test name.
Adding disable-llvm-passes option to make the test more stable as well.
Differential Revision: https://reviews.llvm.org/D89269
In https://reviews.llvm.org/D87470 I added the change to tighten the lifetime of the expression awaiter.await_suspend().address.
Howver it was incorrect. ExprWithCleanups will call the dtor and end the lifetime for all the temps created in the current full expr.
When this is called on a normal await call, we don't want to do that.
We only want to do this for the call on the final_awaiter, to avoid writing into the frame after the frame is destroyed.
This change fixes it, by checking IsImplicit.
Differential Revision: https://reviews.llvm.org/D89066
This reverts commit 55c4ff91bd.
Issues were introduced as discussed in https://reviews.llvm.org/D88241
where this change made previous bugs in the linker and BitCodeWriter
visible.
Make the corresponding change that was made for byval in
b7141207a4. Like byval, this requires a
bulk update of the test IR tests to include the type before this can
be mandatory.
In generating the code for symmetric transfer, a temporary object is created to store the returned handle from await_suspend() call of the awaiter. Previously this temp won't be cleaned up until very later, which ends up causing this temp to be spilled to the heap. However, we know that this temp will no longer be needed after the coro_resume call. We can clean it up right after.
Differential Revision: https://reviews.llvm.org/D87470
Summary:
This patch addresses https://bugs.llvm.org/show_bug.cgi?id=46256
The spec of coroutine requires that the expression co_await promise.final_suspend() shall not be potentially-throwing.
To check this, we recursively look at every call (including Call, MemberCall, OperatorCall and Constructor) in all code
generated by the final suspend, and ensure that the callees are declared with noexcept. We also look at any returned data
type that requires explicit destruction, and check their destructors for noexcept.
This patch does not check declarations with dependent types yet, which will be done in future patches.
Updated all tests to add noexcept to the required functions, and added a dedicated test for this patch.
This patch might start to cause existing codebase fail to compile because most people may not have been strict in tagging
all the related functions noexcept.
Reviewers: lewissbaker, modocache, junparser
Reviewed By: modocache
Subscribers: arphaman, junparser, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D82029
If we're going to assume references are dereferenceable, we should also
assume they're aligned: otherwise, we can't actually dereference them.
See also D80072.
Differential Revision: https://reviews.llvm.org/D80166
Summary:
Right now we annotate C++'s `operator new` with `noalias` attribute,
which very much is healthy for optimizations.
However as per [[ http://eel.is/c++draft/basic.stc.dynamic.allocation | `[basic.stc.dynamic.allocation]` ]],
there are more promises on global `operator new`, namely:
* non-`std::nothrow_t` `operator new` *never* returns `nullptr`
* If `std::align_val_t align` parameter is taken, the pointer will also be `align`-aligned
* ~~global `operator new`-returned pointer is `__STDCPP_DEFAULT_NEW_ALIGNMENT__`-aligned ~~ It's more caveated than that.
Supplying this information may not cause immediate landslide effects
on any specific benchmarks, but it for sure will be healthy for optimizer
in the sense that the IR will better reflect the guarantees provided in the source code.
The caveat is `-fno-assume-sane-operator-new`, which currently prevents emitting `noalias`
attribute, and is automatically passed by Sanitizers ([[ https://bugs.llvm.org/show_bug.cgi?id=16386 | PR16386 ]]) - should it also cover these attributes?
The problem is that the flag is back-end-specific, as seen in `test/Modules/explicit-build-flags.cpp`.
But while it is okay to add `noalias` metadata in backend, we really should be adding at least
the alignment metadata to the AST, since that allows us to perform sema checks on it.
Reviewers: erichkeane, rjmccall, jdoerfert, eugenis, rsmith
Reviewed By: rsmith
Subscribers: xbolva00, jrtc27, atanasyan, nlopes, cfe-commits
Tags: #llvm, #clang
Differential Revision: https://reviews.llvm.org/D73380
Summary:
Depends on https://reviews.llvm.org/D71902.
The last in a series of six patches that ports the LLVM coroutines
passes to the new pass manager infrastructure.
This patch has Clang schedule the new coroutines passes when the
`-fexperimental-new-pass-manager` option is used. With this and the
previous 5 patches, Clang is capable of building and successfully
running the test suite of large coroutines projects such as
https://github.com/lewissbaker/cppcoro with
`ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER=On`.
Reviewers: GorNishanov, lewissbaker, chandlerc, junparser
Subscribers: EricWF, cfe-commits, llvm-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D71903
Summary:
Clang -fpic defaults to -fno-semantic-interposition (GCC -fpic defaults
to -fsemantic-interposition).
Users need to specify -fsemantic-interposition to get semantic
interposition behavior.
Semantic interposition is currently a best-effort feature. There may
still be some cases where it is not handled well.
Reviewers: peter.smith, rnk, serge-sans-paille, sfertile, jfb, jdoerfert
Subscribers: dschuff, jyknight, dylanmckay, nemanjai, jvesely, kbarton, fedor.sergeev, asb, rbar, johnrusso, simoncook, sabuasal, niosHD, jrtc27, zzheng, edward-jones, atanasyan, rogfer01, MartinMosbeck, brucehoult, the_o, arphaman, PkmX, jocewei, jsji, Jim, lenary, s.egerton, pzheng, sameer.abuasal, apazos, luismarques, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D73865
For consistency with normal instructions and clarity when reading IR,
it's best to print the %0, %1, ... names of function arguments in
definitions.
Also modifies the parser to accept IR in that form for obvious reasons.
llvm-svn: 367755
This reverts commit https://reviews.llvm.org/rL344150 which causes
MachineOutliner related failures on the ppc64le multistage buildbot.
llvm-svn: 344526
This is currently a clang extension and a resolution
of the defect report in the C++ Standard.
Differential Revision: https://reviews.llvm.org/D46441
llvm-svn: 344150
Summary:
In his review of https://reviews.llvm.org/D45860, @GorNishanov suggested
avoiding generating additional exception-handling IR in the case that
the resume function was marked as 'noexcept', and exceptions could not
occur. This implements that suggestion.
Test Plan: `check-clang`
Reviewers: GorNishanov, EricWF
Reviewed By: GorNishanov
Subscribers: cfe-commits, GorNishanov
Differential Revision: https://reviews.llvm.org/D47673
llvm-svn: 335422
Summary:
http://wg21.link/P0664r2 section "Evolution/Core Issues 24" describes a
proposed change to Coroutines TS that would have any exceptions thrown
after the initial suspend point of a coroutine be caught by the handler
specified by the promise type's 'unhandled_exception' member function.
This commit provides a sample implementation of the specified behavior.
Test Plan: `check-clang`
Reviewers: GorNishanov, EricWF
Reviewed By: GorNishanov
Subscribers: cfe-commits, lewissbaker, eric_niebler
Differential Revision: https://reviews.llvm.org/D45860
llvm-svn: 331519
A recent addition to Coroutines TS (https://wg21.link/p0913) adds a pre-defined
coroutine noop_coroutine that does nothing. To implement this feature, we implemented
an llvm.coro.noop intrinsic that returns a coroutine handle to a coroutine that
does nothing when resumed or destroyed.
This patch adds a builtin __builtin_coro_noop() that maps to llvm.coro.noop intrinsic.
Related llvm change: https://reviews.llvm.org/D45114
llvm-svn: 328993
Summary:
https://reviews.llvm.org/rL325291 implemented Coroutines TS N4723
section [dcl.fct.def.coroutine]/7, but it performed lookup of allocator
functions within both the global and class scope, whereas the specified
behavior is to perform lookup for custom allocators within just the
class scope.
To fix, add parameters to the `Sema::FindAllocationFunctions` function
such that it can be used to lookup allocators in global scope,
class scope, or both (instead of just being able to look up in just global
scope or in both global and class scope). Then, use those parameters
from within the coroutine Sema.
This incorrect behavior had the unfortunate side-effect of causing the
bug https://bugs.llvm.org/show_bug.cgi?id=36578 (or at least the reports
of that bug in C++ programs). That bug would occur for any C++ user with
a coroutine frame that took a single pointer argument, since it would
then find the global placement form `operator new`, described in the
C++ standard 18.6.1.3.1. This patch prevents Clang from generating code
that triggers the LLVM assert described in that bug report.
Test Plan: `check-clang`
Reviewers: GorNishanov, eric_niebler, lewissbaker
Reviewed By: GorNishanov
Subscribers: EricWF, cfe-commits
Differential Revision: https://reviews.llvm.org/D44552
llvm-svn: 328949
Summary:
Previously we tried too hard to uphold the fiction that destructor
variants work like they do on Itanium throughout the ABI-neutral parts
of clang. This lead to MS C++ ABI incompatiblities and other bugs. Now,
-mconstructor-aliases will no longer control this ABI detail, and clang
-cc1's LLVM IR output will be this much closer to the clang driver's.
Based on a patch by Zahira Ammarguellat:
https://reviews.llvm.org/D39063
I've tried to move the logic that Zahira added into MicrosoftCXXABI.cpp.
There is only one ABI-specific detail sticking out, and that is in
CodeGenModule::getAddrOfCXXStructor, where we collapse complete dtors to
base dtors in the MS ABI.
This fixes PR32990.
Reviewers: erichkeane, zahiraam, majnemer, rjmccall
Subscribers: cfe-commits
Differential Revision: https://reviews.llvm.org/D44505
llvm-svn: 327732
The tests that failed on a windows host have been fixed.
Original message:
Start setting dso_local for COFF.
With this there are still some GVs where we don't set dso_local
because setGVProperties is never called. I intend to fix that in
followup commits. This is just the bare minimum to teach
shouldAssumeDSOLocal what it should do for COFF.
llvm-svn: 325940
With this there are still some GVs where we don't set dso_local
because setGVProperties is never called. I intend to fix that in
followup commits. This is just the bare minimum to teach
shouldAssumeDSOLocal what it should do for COFF.
llvm-svn: 325915
Summary:
Depends on https://reviews.llvm.org/D42605.
An implementation of the behavior described in `[dcl.fct.def.coroutine]/7`:
when a promise type overloads `operator new` using a "placement new"
that takes the same argument types as the coroutine function, that
overload is used when allocating the coroutine frame.
Simply passing references to the coroutine function parameters directly
to `operator new` results in invariant violations in LLVM's coroutine
splitting pass, so this implementation modifies Clang codegen to
produce allocator-specific alloc/store/loads for each parameter being
forwarded to the allocator.
Test Plan: `check-clang`
Reviewers: rsmith, GorNishanov, eric_niebler
Reviewed By: GorNishanov
Subscribers: lewissbaker, EricWF, cfe-commits
Differential Revision: https://reviews.llvm.org/D42606
llvm-svn: 325291
Summary:
Fix NRVO for Gro variable.
Previously, we only marked the GRO declaration as an NRVO variable
when its QualType and the function return's QualType matched exactly
(using operator==). However, this was incorrect for two reasons:
1. We were marking non-class types, such as ints, as being NRVO variables.
2. We failed to handle cases where the canonical types were the same, but the actual `QualType` objects were different. For example, if one was represented by a typedef. (Example: https://godbolt.org/g/3UFgsL)
This patch fixes these bugs by marking the Gro variable as supporting NRVO only
when `BuildReturnStmt` marks the Gro variable as a coroutine candidate.
Reviewers: rsmith, GorNishanov, nicholas
Reviewed By: GorNishanov
Subscribers: majnemer, cfe-commits
Differential Revision: https://reviews.llvm.org/D42343
llvm-svn: 324037
Summary:
Fix NRVO for Gro variable.
Previously, we only marked the GRO declaration as an NRVO variable
when its QualType and the function return's QualType matched exactly
(using operator==). However, this was incorrect for two reasons:
1. We were marking non-class types, such as ints, as being NRVO variables.
2. We failed to handle cases where the canonical types were the same, but the actual `QualType` objects were different. For example, if one was represented by a typedef. (Example: https://godbolt.org/g/3UFgsL)
This patch fixes these bugs by marking the Gro variable as supporting NRVO only
when `BuildReturnStmt` marks the Gro variable as a coroutine candidate.
Reviewers: rsmith, GorNishanov, nicholas
Reviewed By: GorNishanov
Subscribers: majnemer, cfe-commits
Differential Revision: https://reviews.llvm.org/D42343
llvm-svn: 323712
Summary:
Use corutine function arguments to initialize a promise type, but only
if the promise type defines a constructor that takes those arguments.
Otherwise, fall back to the default constructor.
Test Plan: check-clang
Reviewers: rsmith, GorNishanov, eric_niebler
Reviewed By: GorNishanov
Subscribers: toby-allsopp, lewissbaker, EricWF, cfe-commits
Differential Revision: https://reviews.llvm.org/D41820
llvm-svn: 323381
Summary:
We don't want to store cleanup dest slot saved into the coroutine frame (as some of the cleanup code may
access them after coroutine frame destroyed).
This is an alternative to https://reviews.llvm.org/D37093
It is possible to do this for all functions, but, cursory check showed that in -O0, we get slightly longer function (by 1-3 instructions), thus, we are only limiting cleanup.dest.slot elimination to coroutines.
Reviewers: rjmccall, hfinkel, eric_niebler
Reviewed By: eric_niebler
Subscribers: EricWF, cfe-commits
Differential Revision: https://reviews.llvm.org/D39768
llvm-svn: 317981
Summary:
If await_suspend returns a coroutine_handle, as in the example below:
```
coroutine_handle<> await_suspend(coroutine_handle<> h) {
coro.promise().waiter = h;
return coro;
}
```
suspensionExpression processing will resume the coroutine pointed at by that handle.
Related LLVM change rL311751 makes resume calls of this kind `musttail` at any optimization level.
This enables unlimited symmetric control transfer from coroutine to coroutine without blowing up the stack.
Reviewers: GorNishanov
Reviewed By: GorNishanov
Subscribers: rsmith, EricWF, cfe-commits
Differential Revision: https://reviews.llvm.org/D37131
llvm-svn: 311762
Summary:
Previously Clang incorrectly ignored the expression of a void `co_return`. This patch addresses that bug.
I'm not quite sure if I got the code-gen right, but this patch is at least a start.
Reviewers: rsmith, GorNishanov
Reviewed By: rsmith, GorNishanov
Subscribers: cfe-commits
Differential Revision: https://reviews.llvm.org/D36070
llvm-svn: 309545
Summary:
The title says it all.
Reviewers: GorNishanov, rsmith
Reviewed By: GorNishanov
Subscribers: rjmccall, cfe-commits
Differential Revision: https://reviews.llvm.org/D34194
llvm-svn: 305496
Summary:
We were not handling correctly rebuilding of parameter and were not creating copies for them.
Now we will always rebuild parameter moves in TreeTransform's TransformCoroutineBodyStmt.
Reviewers: rsmith, GorNishanov
Reviewed By: rsmith
Subscribers: cfe-commits
Differential Revision: https://reviews.llvm.org/D33797
llvm-svn: 304620
Summary:
Simple types like int are handled by LLVM Coroutines just fine.
But for non-scalar parameters we need to create copies of those parameters in the coroutine frame and make all uses of those parameters to refer to parameter copies.
Reviewers: rsmith, EricWF, GorNishanov
Subscribers: cfe-commits
Differential Revision: https://reviews.llvm.org/D33507
llvm-svn: 303803
Summary:
1. Coroutine cannot be constexpr (added a check in SemaLambda.cpp not to mark coroutine as constexpr)
2. TransformCoroutineBodyStmt should transform ResultDecl and ReturnStmt
Reviewers: rsmith, GorNishanov
Reviewed By: GorNishanov
Subscribers: EricWF, cfe-commits
Differential Revision: https://reviews.llvm.org/D33498
llvm-svn: 303764
Summary:
* Test that coroutine promise destructor is called.
* Test that we call return_void on fallthrough
* Test that we call unhandled exception in a try catch surrounding the body
Reviewers: EricWF, GorNishanov
Reviewed By: GorNishanov
Subscribers: cfe-commits
Differential Revision: https://reviews.llvm.org/D33479
llvm-svn: 303748
Summary:
Sema creates a declaration for gro variable as:
auto $gro = $promise.get_return_object();
However, gro variable has to outlive coroutine frame and coroutine promise, but,
it can only be initialized after the coroutine promise was created, thus, we
split its emission in two parts: EmitGroAlloca emits an alloca and sets up
the cleanups. Later when the coroutine promise is available we initialize
the gro and set the flag that the cleanup is now active.
Duplicate of: https://reviews.llvm.org/D31670 (which arc patch refuses to apply for some reason)
Reviewers: GorNishanov, rsmith
Reviewed By: GorNishanov
Subscribers: EricWF, cfe-commits
Differential Revision: https://reviews.llvm.org/D33477
llvm-svn: 303716
Arthur Eubanks