Skip to content

[region-isolation] Fixes that enable the stdlib to build with region based isolation #71429

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Feb 7, 2024

Conversation

gottesmm
Copy link
Contributor

@gottesmm gottesmm commented Feb 7, 2024

Decided to use the stdlib to test out region based isolation. Found some issues. They are:

  1. I discovered in a bunch of cases, we cannot rely on the AST in a real way to determine argument locations around for in loops. Rather than fix that AST, I just did the right thing and made it so that in those cases we emit a diagnostic that infers the name of the value from SIL rather than a type from the AST. I refactored code from the MoveChecker for this purpose and I also added the ability to test the VariableNameInferrer (the utilities name) independently of both by using specify_test.
  2. I found I was not handling a few instructions correctly. I updated them.
  3. We were transferring @out parameters in certain cases rather than treating them as initializing a value. Of course in cases where it is illegal to use such a value Sema will error. But we should handle it correctly.
  4. There is a part of MemAccessUtils that as a hack until we have opaque values treat (project_box (load %boxAddr)) as a projection. I assumed we would never see a project_box as a projection and asserted. We hit that assert in the stdlib.

….cpp -> VariableNameUtils.

I am going to reuse this for TransferNonSendable. In the process I made a few
changes to make it more amenable to both use cases and used the current set of
tests that we have for noncopyable types to validate that I didn't break
anything.
…n possible.

A name diagnostic looks as follows:

Some notes:

1. VariableNameUtils still doesn't pattern match all cases. In the cases where
we fail to pattern match, I fall back to the old diagnostics. I am going to be
adding tests/etc specific to VariableNameUtils in a later patch.

2. I took this as an opportunity to begin preparing to change the diagnostics
into diags of the following form:

a. The main diagnostic is changed to something short "transferring non-Sendable
%0 could yield races with later accesses".

b. I added a longer note at the same location that explains that our caller is
in a specific isolation domain and our callee is isolated differently.

c. I added a note on the definition location of the identified value.
I was correct that it is lookthrough, but it has multiple parameters, so the
normal "baked" implementation cannot be used since the "baked" implementation
assumes that any insts it handles has a single operand.
…r, so we need to treat it as a require.

We should make this as a transfer... but this is just temporary until I get
around to it. For now this is always an out parameter, so we should be safe.
project_box's type is not a box type... it is the type contained in the box.
I thought that we would never actually hit this test case, but since we do not
have opaque values this pattern is treated like a projection in certain cases. I
updated the code as appropriate and added a test.
Instead treat them as actual results.
@gottesmm
Copy link
Contributor Author

gottesmm commented Feb 7, 2024

@swift-ci smoke test

This is because in 1e4fe6c, we stopped
transfering the function callee as well.
@gottesmm
Copy link
Contributor Author

gottesmm commented Feb 7, 2024

@swift-ci smoke test

@gottesmm gottesmm enabled auto-merge February 7, 2024 06:54
@gottesmm
Copy link
Contributor Author

gottesmm commented Feb 7, 2024

@swift-ci smoke test linux platform

@gottesmm
Copy link
Contributor Author

gottesmm commented Feb 7, 2024

@swift-ci smoke test macOS platform

@gottesmm
Copy link
Contributor Author

gottesmm commented Feb 7, 2024

@swift-ci smoke test linux platform

@gottesmm
Copy link
Contributor Author

gottesmm commented Feb 7, 2024

macOS platform is failing due to something unrelated in IRGen.

@gottesmm
Copy link
Contributor Author

gottesmm commented Feb 7, 2024

Looks like they all failed:

/home/build-user/swift/lib/IRGen/GenCall.cpp:5853:52: error: no member named 'coro_end_results' in namespace 'llvm::Intrinsic'; did you mean 'coro_end_async'?
      Builder.CreateIntrinsicCall(llvm::Intrinsic::coro_end_results, args);
                                  ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~
                                                   coro_end_async
/home/build-user/build/buildbot_linux/llvm-linux-x86_64/include/llvm/IR/IntrinsicEnums.inc:44:5: note: 'coro_end_async' declared here
    coro_end_async,                            // llvm.coro.end.async
    ^

@hborla
Copy link
Member

hborla commented Feb 7, 2024

@swift-ci please smoke test

@gottesmm gottesmm merged commit 1795f7f into swiftlang:main Feb 7, 2024
@gottesmm gottesmm deleted the fixes-for-stdlib branch February 7, 2024 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants