Skip to content
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

libunwind changes needed to run code in sgx environment via rust-sgx. #57

Merged
merged 1 commit into from
Jun 2, 2020
Merged

libunwind changes needed to run code in sgx environment via rust-sgx. #57

merged 1 commit into from
Jun 2, 2020

Conversation

AdrianCX
Copy link

@AdrianCX AdrianCX commented May 5, 2020

Please advise on process as this is the first PR against rust-lang's llvm and I tried to follow instructions as close as possible..

Ticket: fortanix/rust-sgx#174

Description: libunwind changes needed to run code in sgx environment via rust-sgx.

Target that uses this in rust: x86_64-fortanix-unknown-sgx.
Rust-lang PR depends on this - plan is to raise it if this PR is accepted.

Without this change, rust std for this toolchain is forced to use a precompiled library loaded via environment variable.

Code is guarded via defines to enable only if 'RUST_SGX' is present.

Main logic is in libunwind/src/AddressSpace.hpp

We use 6 symbols to figure out where eh_frame / eh_frame_hdr is at runtime when loaded in an SGX enclave. (EH symbols + IMAGE base)
These are set when elf is converged to sgx format by 'fortanix-sgx-tools'.

Original ported changes: llvm/llvm-project@release/5.x...fortanix:release/5.x

@cuviper
Copy link
Member

cuviper commented May 5, 2020

Is it possible to reframe this in a way that upstream LLVM would accept? We don't generally want to carry extra patches indefinitely. With backports, at least we know they'll go away in a future rebase.

@jethrogb
Copy link

jethrogb commented May 5, 2020

We don't generally want to carry extra patches indefinitely.

The Rust project effectively already does this by pointing at a a fork. That fork is based on LLVM version 5. Compared to using that external dependency, this PR is a large step in the right direction of bringing us much closer to upstream.

libunwind/src/UnwindRustSgxSnprintf.c Outdated Show resolved Hide resolved
libunwind/src/libunwind.cpp Outdated Show resolved Hide resolved
libunwind/src/UnwindRustSgx.h Outdated Show resolved Hide resolved
@cuviper
Copy link
Member

cuviper commented May 25, 2020

Please note that we've moved to LLVM 10 since rust-lang/rust#67759, so we'll need this PR rebased onto rustc/10.0-2020-05-05 to reach rust master.

Code is guarded via defines to enable only if 'RUST_SGX' is present.

Main logic is in libunwind/src/AddressSpace.hpp
We use 6 symbols to figure out where eh_frame / eh_frame_hdr is at runtime when loaded in an SGX enclave. (EH symbols + IMAGE base)
These are set by 'fortanix-sgx-tools'.

As notes:
- Target above at the moment uses a pre-compiled libunwind.a from forked repo.
- Goal of these changes is to use official llvm with patch.
- Changes in rust-lang to use this are planned if/when this is accepted.
- Ticket: fortanix/rust-sgx#174
- Original ported changes: llvm/llvm-project@release/5.x...fortanix:release/5.x
@AdrianCX
Copy link
Author

AdrianCX commented May 26, 2020

Please note that we've moved to LLVM 10 since rust-lang/rust#67759, so we'll need this PR rebased onto rustc/10.0-2020-05-05 to reach rust master.

Thanks, I updated this PR with required changes and modified target.

@AdrianCX AdrianCX changed the base branch from rustc/9.0-2019-12-19 to rustc/10.0-2020-05-05 May 26, 2020 07:37
@cuviper
Copy link
Member

cuviper commented Jun 2, 2020

Ok, I'm going to trust @jethrogb's review for SGX's sake, and this looks harmless to other targets.

@cuviper cuviper merged commit 5992729 into rust-lang:rustc/10.0-2020-05-05 Jun 2, 2020
@AdrianCX AdrianCX deleted the rustc/9.0-2019-12-19-rust-sgx-libunwind branch June 2, 2020 07:05
Manishearth added a commit to Manishearth/rust that referenced this pull request Jun 26, 2020
Fortanix SGX target libunwind build process changes

Ticket: fortanix/rust-sgx#174
LLVM related changes (merged): rust-lang/llvm-project#57

Description: libunwind changes needed to run code in sgx environment via rust-sgx.

Target that uses this in rust: x86_64-fortanix-unknown-sgx.

Without this change, rust std for this toolchain is forced to use a precompiled library loaded via environment variable.

With this change we act the same as musl target.
nikic pushed a commit to nikic/llvm-project that referenced this pull request May 14, 2023
…callback

The `TypeSystemMap::m_mutex` guards against concurrent modifications
of members of `TypeSystemMap`. In particular, `m_map`.

`TypeSystemMap::ForEach` iterates through the entire `m_map` calling
a user-specified callback for each entry. This is all done while
`m_mutex` is locked. However, there's nothing that guarantees that
the callback itself won't call back into `TypeSystemMap` APIs on the
same thread. This lead to double-locking `m_mutex`, which is undefined
behaviour. We've seen this cause a deadlock in the swift plugin with
following backtrace:

```

int main() {
    std::unique_ptr<int> up = std::make_unique<int>(5);

    volatile int val = *up;
    return val;
}

clang++ -std=c++2a -g -O1 main.cpp

./bin/lldb -o “br se -p return” -o run -o “v *up” -o “expr *up” -b
```

```
frame rust-lang#4: std::lock_guard<std::mutex>::lock_guard
frame rust-lang#5: lldb_private::TypeSystemMap::GetTypeSystemForLanguage <<<< Lock rust-lang#2
frame rust-lang#6: lldb_private::TypeSystemMap::GetTypeSystemForLanguage
frame rust-lang#7: lldb_private::Target::GetScratchTypeSystemForLanguage
...
frame rust-lang#26: lldb_private::SwiftASTContext::LoadLibraryUsingPaths
frame rust-lang#27: lldb_private::SwiftASTContext::LoadModule
frame rust-lang#30: swift::ModuleDecl::collectLinkLibraries
frame rust-lang#31: lldb_private::SwiftASTContext::LoadModule
frame rust-lang#34: lldb_private::SwiftASTContext::GetCompileUnitImportsImpl
frame rust-lang#35: lldb_private::SwiftASTContext::PerformCompileUnitImports
frame rust-lang#36: lldb_private::TypeSystemSwiftTypeRefForExpressions::GetSwiftASTContext
frame rust-lang#37: lldb_private::TypeSystemSwiftTypeRefForExpressions::GetPersistentExpressionState
frame rust-lang#38: lldb_private::Target::GetPersistentSymbol
frame rust-lang#41: lldb_private::TypeSystemMap::ForEach                 <<<< Lock #1
frame rust-lang#42: lldb_private::Target::GetPersistentSymbol
frame rust-lang#43: lldb_private::IRExecutionUnit::FindInUserDefinedSymbols
frame rust-lang#44: lldb_private::IRExecutionUnit::FindSymbol
frame rust-lang#45: lldb_private::IRExecutionUnit::MemoryManager::GetSymbolAddressAndPresence
frame rust-lang#46: lldb_private::IRExecutionUnit::MemoryManager::findSymbol
frame rust-lang#47: non-virtual thunk to lldb_private::IRExecutionUnit::MemoryManager::findSymbol
frame rust-lang#48: llvm::LinkingSymbolResolver::findSymbol
frame rust-lang#49: llvm::LegacyJITSymbolResolver::lookup
frame rust-lang#50: llvm::RuntimeDyldImpl::resolveExternalSymbols
frame rust-lang#51: llvm::RuntimeDyldImpl::resolveRelocations
frame rust-lang#52: llvm::MCJIT::finalizeLoadedModules
frame rust-lang#53: llvm::MCJIT::finalizeObject
frame rust-lang#54: lldb_private::IRExecutionUnit::ReportAllocations
frame rust-lang#55: lldb_private::IRExecutionUnit::GetRunnableInfo
frame rust-lang#56: lldb_private::ClangExpressionParser::PrepareForExecution
frame rust-lang#57: lldb_private::ClangUserExpression::TryParse
frame rust-lang#58: lldb_private::ClangUserExpression::Parse
```

Our solution is to simply iterate over a local copy of `m_map`.

**Testing**

* Confirmed on manual reproducer (would reproduce 100% of the time
  before the patch)

Differential Revision: https://reviews.llvm.org/D149949
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.

3 participants