[llvm] 1caabbe - [OpaquePtr] Fix initialization-order-fiasco
Vitaly Buka via llvm-commits
llvm-commits at lists.llvm.org
Sun Nov 7 17:39:40 PST 2021
Sorry, I wrongly assumed it's obvious.
D112732 accidentally fixed the unnoticed 10 month old regression in the
initialization-order-fiasco check. So some bots started to fail with:
```
=================================================================
==3497602==ERROR: AddressSanitizer: initialization-order-fiasco on address
0x000001b238c8 at pc 0x000000de6350 bp 0x7ffe74e4f590 sp 0x7ffe74e4f588
READ of size 1 at 0x000001b238c8 thread T0
#0 0xde634f in getValue
/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/include/llvm/Support/CommandLine.h:1417:38
#1 0xde634f in operator bool
/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/include/llvm/Support/CommandLine.h:1421:38
#2 0xde634f in
llvm::LLVMContextImpl::LLVMContextImpl(llvm::LLVMContext&)
/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/lib/IR/LLVMContextImpl.cpp:39:22
#3 0xda6b8e in llvm::LLVMContext::LLVMContext()
/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/lib/IR/LLVMContext.cpp:36:40
#4 0x82a941 in __cxx_global_var_init
/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/tools/llvm-lipo/llvm-lipo.cpp:37:20
#5 0x82a941 in _GLOBAL__sub_I_llvm_lipo.cpp
/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/tools/llvm-lipo/llvm-lipo.cpp
#6 0x7f7b72c8a0fc in __libc_start_main
(/lib/x86_64-linux-gnu/libc.so.6+0x2e0fc)
#7 0x71be34 in _start
(/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/bin/llvm-lipo+0x71be34)
```
https://lab.llvm.org/staging/#/builders/157/builds/231/steps/10/logs/stdio
Constructors of globals of type LLVMContext were reading content of another
global OpaquePointersCL before it was constructed. So the main point was to
remove OpaquePointersCL from LLVMContextImpl constructor.
The rest is just to preserve behaviour with a minimal patch.
set/get were added to make OpaquePointers private to prevent users
accessing them without checking OpaquePointersCL.
enableOpaquePointers and supportsTypedPointers can be removed if we OK that
users of LLVMContext can also look into LLVMContextImpl.
BTW. without disableOpaquePointers OpaquePointers can be just bool as
before and:
bool getOpaquePointers() {
return OpaquePointers || OpaquePointersCL;
}
On Fri, 5 Nov 2021 at 01:15, Nikita Popov <nikita.ppv at gmail.com> wrote:
> On Fri, Nov 5, 2021 at 3:29 AM Vitaly Buka via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>>
>> Author: Vitaly Buka
>> Date: 2021-11-04T19:29:06-07:00
>> New Revision: 1caabbef8e8e73b3dd0cf1f15cf7417d75b7621c
>>
>> URL:
>> https://github.com/llvm/llvm-project/commit/1caabbef8e8e73b3dd0cf1f15cf7417d75b7621c
>> DIFF:
>> https://github.com/llvm/llvm-project/commit/1caabbef8e8e73b3dd0cf1f15cf7417d75b7621c.diff
>>
>> LOG: [OpaquePtr] Fix initialization-order-fiasco
>>
>> Asan detects it after D112732.
>>
>
> Can you please explain why this change is needed? (It would have been nice
> if that explanation was part of the commit message...)
>
> Why do we now have both enableOpaquePointers() and setOpaquePointers(), as
> well as supportsTypedPointers() and getOpaquePointers()? These method pairs
> look redundant to me.
>
> Regards,
> Nikita
>
> Added:
>>
>>
>> Modified:
>> llvm/lib/IR/LLVMContext.cpp
>> llvm/lib/IR/LLVMContextImpl.cpp
>> llvm/lib/IR/LLVMContextImpl.h
>> llvm/lib/IR/Type.cpp
>>
>> Removed:
>>
>>
>>
>>
>> ################################################################################
>> diff --git a/llvm/lib/IR/LLVMContext.cpp b/llvm/lib/IR/LLVMContext.cpp
>> index dce5d17c9eea..90716d9c81a6 100644
>> --- a/llvm/lib/IR/LLVMContext.cpp
>> +++ b/llvm/lib/IR/LLVMContext.cpp
>> @@ -351,9 +351,9 @@ std::unique_ptr<DiagnosticHandler>
>> LLVMContext::getDiagnosticHandler() {
>> void LLVMContext::enableOpaquePointers() const {
>> assert(pImpl->PointerTypes.empty() && pImpl->ASPointerTypes.empty() &&
>> "Must be called before creating any pointer types");
>> - pImpl->OpaquePointers = true;
>> + pImpl->setOpaquePointers(true);
>> }
>>
>> bool LLVMContext::supportsTypedPointers() const {
>> - return !pImpl->OpaquePointers;
>> + return !pImpl->getOpaquePointers();
>> }
>>
>> diff --git a/llvm/lib/IR/LLVMContextImpl.cpp
>> b/llvm/lib/IR/LLVMContextImpl.cpp
>> index 068bc58b6796..ebbf382aea38 100644
>> --- a/llvm/lib/IR/LLVMContextImpl.cpp
>> +++ b/llvm/lib/IR/LLVMContextImpl.cpp
>> @@ -35,8 +35,7 @@ LLVMContextImpl::LLVMContextImpl(LLVMContext &C)
>> X86_FP80Ty(C, Type::X86_FP80TyID), FP128Ty(C, Type::FP128TyID),
>> PPC_FP128Ty(C, Type::PPC_FP128TyID), X86_MMXTy(C,
>> Type::X86_MMXTyID),
>> X86_AMXTy(C, Type::X86_AMXTyID), Int1Ty(C, 1), Int8Ty(C, 8),
>> - Int16Ty(C, 16), Int32Ty(C, 32), Int64Ty(C, 64), Int128Ty(C, 128),
>> - OpaquePointers(OpaquePointersCL) {}
>> + Int16Ty(C, 16), Int32Ty(C, 32), Int64Ty(C, 64), Int128Ty(C, 128) {}
>>
>> LLVMContextImpl::~LLVMContextImpl() {
>> // NOTE: We need to delete the contents of OwnedModules, but Module's
>> dtor
>> @@ -233,3 +232,11 @@ OptPassGate &LLVMContextImpl::getOptPassGate() const
>> {
>> void LLVMContextImpl::setOptPassGate(OptPassGate& OPG) {
>> this->OPG = &OPG;
>> }
>> +
>> +bool LLVMContextImpl::getOpaquePointers() {
>> + if (LLVM_UNLIKELY(!(OpaquePointers.hasValue())))
>> + OpaquePointers = OpaquePointersCL;
>> + return *OpaquePointers;
>> +}
>> +
>> +void LLVMContextImpl::setOpaquePointers(bool OP) { OpaquePointers = OP; }
>>
>> diff --git a/llvm/lib/IR/LLVMContextImpl.h
>> b/llvm/lib/IR/LLVMContextImpl.h
>> index b17f581faaa6..d84714d9b1f1 100644
>> --- a/llvm/lib/IR/LLVMContextImpl.h
>> +++ b/llvm/lib/IR/LLVMContextImpl.h
>> @@ -1461,10 +1461,7 @@ class LLVMContextImpl {
>> unsigned NamedStructTypesUniqueID = 0;
>>
>> DenseMap<std::pair<Type *, uint64_t>, ArrayType*> ArrayTypes;
>> - DenseMap<std::pair<Type *, ElementCount>, VectorType*> VectorTypes;
>> - // TODO: clean up the following after we no longer support non-opaque
>> pointer
>> - // types.
>> - bool OpaquePointers;
>> + DenseMap<std::pair<Type *, ElementCount>, VectorType *> VectorTypes;
>> DenseMap<Type*, PointerType*> PointerTypes; // Pointers in AddrSpace
>> = 0
>> DenseMap<std::pair<Type*, unsigned>, PointerType*> ASPointerTypes;
>>
>> @@ -1544,6 +1541,14 @@ class LLVMContextImpl {
>> /// The lifetime of the object must be guaranteed to extend as long as
>> the
>> /// LLVMContext is used by compilation.
>> void setOptPassGate(OptPassGate&);
>> +
>> + // TODO: clean up the following after we no longer support non-opaque
>> pointer
>> + // types.
>> + bool getOpaquePointers();
>> + void setOpaquePointers(bool OP);
>> +
>> +private:
>> + Optional<bool> OpaquePointers;
>> };
>>
>> } // end namespace llvm
>>
>> diff --git a/llvm/lib/IR/Type.cpp b/llvm/lib/IR/Type.cpp
>> index 0a28a001ef0d..d59d87ad631b 100644
>> --- a/llvm/lib/IR/Type.cpp
>> +++ b/llvm/lib/IR/Type.cpp
>> @@ -733,7 +733,7 @@ PointerType *PointerType::get(Type *EltTy, unsigned
>> AddressSpace) {
>> LLVMContextImpl *CImpl = EltTy->getContext().pImpl;
>>
>> // Automatically convert typed pointers to opaque pointers.
>> - if (CImpl->OpaquePointers)
>> + if (CImpl->getOpaquePointers())
>> return get(EltTy->getContext(), AddressSpace);
>>
>> // Since AddressSpace #0 is the common case, we special case it.
>> @@ -747,7 +747,7 @@ PointerType *PointerType::get(Type *EltTy, unsigned
>> AddressSpace) {
>>
>> PointerType *PointerType::get(LLVMContext &C, unsigned AddressSpace) {
>> LLVMContextImpl *CImpl = C.pImpl;
>> - assert(CImpl->OpaquePointers &&
>> + assert(CImpl->getOpaquePointers() &&
>> "Can only create opaque pointers in opaque pointer mode");
>>
>> // Since AddressSpace #0 is the common case, we special case it.
>>
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20211107/97cf8c2b/attachment.html>
More information about the llvm-commits
mailing list