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