From e7a2ea281114e9feb9914bb7684932e53852b4cc Mon Sep 17 00:00:00 2001 From: Mike Ash Date: Tue, 25 Aug 2020 16:48:31 -0400 Subject: [PATCH 1/5] Fix GetTypeInfo for changes to decodeMangledType in Swift. --- .../source/Target/SwiftLanguageRuntimeDynamicTypeResolution.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lldb/source/Target/SwiftLanguageRuntimeDynamicTypeResolution.cpp b/lldb/source/Target/SwiftLanguageRuntimeDynamicTypeResolution.cpp index 5ff30e9289376..936f939d8a06c 100644 --- a/lldb/source/Target/SwiftLanguageRuntimeDynamicTypeResolution.cpp +++ b/lldb/source/Target/SwiftLanguageRuntimeDynamicTypeResolution.cpp @@ -1729,7 +1729,7 @@ SwiftLanguageRuntimeImpl::GetTypeInfo(CompilerType type) { swift::Demangle::Demangler Dem; auto demangled = Dem.demangleType(mangled_no_prefix); auto *type_ref = swift::Demangle::decodeMangledType( - reflection_ctx->getBuilder(), demangled); + reflection_ctx->getBuilder(), demangled).getType(); if (!type_ref) return nullptr; return reflection_ctx->getBuilder().getTypeConverter().getTypeInfo(type_ref); From d5efdad5254dcd986be12b680d89af4ab17efbbb Mon Sep 17 00:00:00 2001 From: Florian Hahn Date: Fri, 21 Aug 2020 16:10:02 +0100 Subject: [PATCH 2/5] [Constants] Handle FNeg in getWithOperands. Currently ConstantExpr::getWithOperands does not handle FNeg and subsequently treats FNeg as binary operator, leading to an assertion failure or segmentation fault if built without assertions. Originally I reproduced this with llvm-dis on a bitcode file, which I unfortunately cannot share and also cannot really reduce. But PR45426 describes the same issue and has a reproducer with Clang, so I'll go with that. Reviewed By: aprantl Differential Revision: https://reviews.llvm.org/D86274 (Cherry-picked from bc72a3ab949e14b990c080985fc1e74475f1e7d2) --- clang/test/CodeGen/constantexpr-fneg.c | 20 ++++++++++++++++++++ llvm/lib/IR/Constants.cpp | 2 ++ 2 files changed, 22 insertions(+) create mode 100644 clang/test/CodeGen/constantexpr-fneg.c diff --git a/clang/test/CodeGen/constantexpr-fneg.c b/clang/test/CodeGen/constantexpr-fneg.c new file mode 100644 index 0000000000000..57f6164375166 --- /dev/null +++ b/clang/test/CodeGen/constantexpr-fneg.c @@ -0,0 +1,20 @@ +// RUN: %clang_cc1 -emit-llvm-bc -disable-llvm-passes -o %t.bc %s +// RUN: llvm-dis %t.bc -o - | FileCheck %s + +// Test case for PR45426. Make sure we do not crash while writing bitcode +// containing a simplify-able fneg constant expression. Check that the created +// bitcode file can be disassembled and has the constant expressions simplified. +// +// CHECK-LABEL define i32 @main() +// CHECK: entry: +// CHECK-NEXT: %retval = alloca i32 +// CHECK-NEXT: store i32 0, i32* %retval +// CHECK-NEXT: [[LV:%.*]] = load float*, float** @c +// CHECK-NEXT: store float 1.000000e+00, float* [[LV]], align 4 +// CHECK-NEXT: ret i32 -1 + +int a[], b; +float *c; +int main() { + return -(*c = &b != a); +} diff --git a/llvm/lib/IR/Constants.cpp b/llvm/lib/IR/Constants.cpp index cafb412b795b2..06861aa2b1094 100644 --- a/llvm/lib/IR/Constants.cpp +++ b/llvm/lib/IR/Constants.cpp @@ -1316,6 +1316,8 @@ Constant *ConstantExpr::getWithOperands(ArrayRef Ops, Type *Ty, OnlyIfReducedTy); case Instruction::ExtractValue: return ConstantExpr::getExtractValue(Ops[0], getIndices(), OnlyIfReducedTy); + case Instruction::FNeg: + return ConstantExpr::getFNeg(Ops[0]); case Instruction::ShuffleVector: return ConstantExpr::getShuffleVector(Ops[0], Ops[1], Ops[2], OnlyIfReducedTy); From 762d17e112ba90e677daef60fafaa82236f28064 Mon Sep 17 00:00:00 2001 From: Saleem Abdulrasool Date: Thu, 27 Aug 2020 14:20:35 -0700 Subject: [PATCH 3/5] Support: add workaround for newer MSVC toolchains The MSVC toolchain implemented a resolution to DR1734, which results in `llvm::is_trivially_copyable` to not match with `std::is_trivially_copyable`. This is the minimal change that will match the ABI and allow building LLVM with a newer MSVC toolchain. --- llvm/include/llvm/Support/type_traits.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/llvm/include/llvm/Support/type_traits.h b/llvm/include/llvm/Support/type_traits.h index b7d48e8e1ade5..12019bfc5a6c6 100644 --- a/llvm/include/llvm/Support/type_traits.h +++ b/llvm/include/llvm/Support/type_traits.h @@ -186,6 +186,13 @@ template class is_trivially_copyable : public std::true_type { }; +// Workaround the resolution to DR1734 +#if _MSVC_STL_UPDATE-0l > 202002l +template <> +class is_trivially_copyable> : public std::true_type { +}; +#endif + } // end namespace llvm From 8ef72209120513dea8a021fc3acac2dbc0aebec2 Mon Sep 17 00:00:00 2001 From: Robert Widmann Date: Fri, 28 Aug 2020 15:48:02 -0700 Subject: [PATCH 4/5] Revert "Increase the Entropy of ModuleManager Map Keys" This reverts commit 7fb8148f23e153d4bde892c71a5ef0b80411329d. --- .../clang/Serialization/ModuleManager.h | 46 +------------------ clang/lib/Serialization/ModuleManager.cpp | 12 ++--- 2 files changed, 8 insertions(+), 50 deletions(-) diff --git a/clang/include/clang/Serialization/ModuleManager.h b/clang/include/clang/Serialization/ModuleManager.h index 3c5ccb1d88fb0..15ddb9875ff17 100644 --- a/clang/include/clang/Serialization/ModuleManager.h +++ b/clang/include/clang/Serialization/ModuleManager.h @@ -59,50 +59,8 @@ class ModuleManager { // to implement short-circuiting logic when running DFS over the dependencies. SmallVector Roots; - /// An \c EntryKey is a thin wrapper around a \c FileEntry that implements - /// a richer notion of identity. - /// - /// A plain \c FileEntry has its identity tied to inode numbers. When the - /// module cache regenerates a PCM, some filesystem allocators may reuse - /// inode numbers for distinct modules, which can cause the cache to return - /// mismatched entries. An \c EntryKey ensures that the size and modification - /// time are taken into account when determining the identity of a key, which - /// significantly decreases - but does not eliminate - the chance of - /// a collision. - struct EntryKey { - const FileEntry *Entry; - - struct Info { - static inline EntryKey getEmptyKey() { - return EntryKey{llvm::DenseMapInfo::getEmptyKey()}; - } - static inline EntryKey getTombstoneKey() { - return EntryKey{ - llvm::DenseMapInfo::getTombstoneKey()}; - } - static unsigned getHashValue(const EntryKey &Val) { - return llvm::DenseMapInfo::getHashValue(Val.Entry); - } - static bool isEqual(const EntryKey &LHS, const EntryKey &RHS) { - if (LHS.Entry == getEmptyKey().Entry || - LHS.Entry == getTombstoneKey().Entry || - RHS.Entry == getEmptyKey().Entry || - RHS.Entry == getTombstoneKey().Entry) { - return LHS.Entry == RHS.Entry; - } - if (LHS.Entry == nullptr || RHS.Entry == nullptr) { - return LHS.Entry == RHS.Entry; - } - return LHS.Entry == RHS.Entry && - LHS.Entry->getSize() == RHS.Entry->getSize() && - LHS.Entry->getModificationTime() == - RHS.Entry->getModificationTime(); - } - }; - }; - /// All loaded modules, indexed by name. - llvm::DenseMap Modules; + llvm::DenseMap Modules; /// FileManager that handles translating between filenames and /// FileEntry *. @@ -118,7 +76,7 @@ class ModuleManager { const HeaderSearch &HeaderSearchInfo; /// A lookup of in-memory (virtual file) buffers - llvm::DenseMap, EntryKey::Info> + llvm::DenseMap> InMemoryBuffers; /// The visitation order. diff --git a/clang/lib/Serialization/ModuleManager.cpp b/clang/lib/Serialization/ModuleManager.cpp index f5d83964b90ff..94d64c125db3e 100644 --- a/clang/lib/Serialization/ModuleManager.cpp +++ b/clang/lib/Serialization/ModuleManager.cpp @@ -59,7 +59,7 @@ ModuleFile *ModuleManager::lookupByModuleName(StringRef Name) const { } ModuleFile *ModuleManager::lookup(const FileEntry *File) const { - auto Known = Modules.find(EntryKey{File}); + auto Known = Modules.find(File); if (Known == Modules.end()) return nullptr; @@ -72,7 +72,7 @@ ModuleManager::lookupBuffer(StringRef Name) { /*CacheFailure=*/false); if (!Entry) return nullptr; - return std::move(InMemoryBuffers[EntryKey{*Entry}]); + return std::move(InMemoryBuffers[*Entry]); } static bool checkSignature(ASTFileSignature Signature, @@ -133,7 +133,7 @@ ModuleManager::addModule(StringRef FileName, ModuleKind Type, } // Check whether we already loaded this module, before - if (ModuleFile *ModuleEntry = Modules.lookup(EntryKey{Entry})) { + if (ModuleFile *ModuleEntry = Modules.lookup(Entry)) { // Check the stored signature. if (checkSignature(ModuleEntry->Signature, ExpectedSignature, ErrorStr)) return OutOfDate; @@ -208,7 +208,7 @@ ModuleManager::addModule(StringRef FileName, ModuleKind Type, return OutOfDate; // We're keeping this module. Store it everywhere. - Module = Modules[EntryKey{Entry}] = NewModule.get(); + Module = Modules[Entry] = NewModule.get(); updateModuleImports(*NewModule, ImportedBy, ImportLoc); @@ -255,7 +255,7 @@ void ModuleManager::removeModules(ModuleIterator First, ModuleMap *modMap) { // Delete the modules and erase them from the various structures. for (ModuleIterator victim = First; victim != Last; ++victim) { - Modules.erase(EntryKey{victim->File}); + Modules.erase(victim->File); if (modMap) { StringRef ModuleName = victim->ModuleName; @@ -274,7 +274,7 @@ ModuleManager::addInMemoryBuffer(StringRef FileName, std::unique_ptr Buffer) { const FileEntry *Entry = FileMgr.getVirtualFile(FileName, Buffer->getBufferSize(), 0); - InMemoryBuffers[EntryKey{Entry}] = std::move(Buffer); + InMemoryBuffers[Entry] = std::move(Buffer); } ModuleManager::VisitState *ModuleManager::allocateVisitState() { From 01595d7eb157581436795a43423c4a740ef89fe3 Mon Sep 17 00:00:00 2001 From: Robert Widmann Date: Fri, 28 Aug 2020 15:57:48 -0700 Subject: [PATCH 5/5] Check Implicit Module Names After Cache Hits The ModuleManager's use of FileEntry nodes as the keys for its map of loaded modules is less than ideal. Uniqueness for FileEntry nodes is maintained by FileManager, which in turn uses inode numbers on hosts that support that. When coupled with the module cache's proclivity for turning over and deleting stale PCMs, this means entries for different module files can wind up reusing the same underlying inode. When this happens, subsequent accesses to the Modules map will disagree on the ModuleFile associated with a given file. In general, it is not sufficient to resolve this conundrum with a type like FileEntryRef that stores the name of the FileEntry node on first access because of path canonicalization issues. However, the paths constructed for implicit module builds are fully under Clang's control. We *can*, therefore, rely on their structure being consistent across operating systems and across subsequent accesses to the Modules map. To mitigate the effects of inode reuse, perform an extra name check when implicit modules are returned from the cache. This has the effect of forcing reused FileEntry nodes to stomp over existing-but-stale entries in the cache, which simulates a miss - exactly the desired behavior. rdar://48443680 --- clang/lib/Serialization/ModuleManager.cpp | 37 ++++++++++++++++++----- 1 file changed, 30 insertions(+), 7 deletions(-) diff --git a/clang/lib/Serialization/ModuleManager.cpp b/clang/lib/Serialization/ModuleManager.cpp index 94d64c125db3e..43d49e1ad45b7 100644 --- a/clang/lib/Serialization/ModuleManager.cpp +++ b/clang/lib/Serialization/ModuleManager.cpp @@ -132,15 +132,38 @@ ModuleManager::addModule(StringRef FileName, ModuleKind Type, return Missing; } + // The ModuleManager's use of FileEntry nodes as the keys for its map of + // loaded modules is less than ideal. Uniqueness for FileEntry nodes is + // maintained by FileManager, which in turn uses inode numbers on hosts + // that support that. When coupled with the module cache's proclivity for + // turning over and deleting stale PCMs, this means entries for different + // module files can wind up reusing the same underlying inode. When this + // happens, subsequent accesses to the Modules map will disagree on the + // ModuleFile associated with a given file. In general, it is not sufficient + // to resolve this conundrum with a type like FileEntryRef that stores the + // name of the FileEntry node on first access because of path canonicalization + // issues. However, the paths constructed for implicit module builds are + // fully under Clang's control. We *can*, therefore, rely on their structure + // being consistent across operating systems and across subsequent accesses + // to the Modules map. + auto implicitModuleNamesMatch = [](ModuleKind Kind, const ModuleFile *MF, + const FileEntry *Entry) -> bool { + if (Kind != MK_ImplicitModule) + return true; + return Entry->getName() == MF->FileName; + }; + // Check whether we already loaded this module, before if (ModuleFile *ModuleEntry = Modules.lookup(Entry)) { - // Check the stored signature. - if (checkSignature(ModuleEntry->Signature, ExpectedSignature, ErrorStr)) - return OutOfDate; - - Module = ModuleEntry; - updateModuleImports(*ModuleEntry, ImportedBy, ImportLoc); - return AlreadyLoaded; + if (implicitModuleNamesMatch(Type, ModuleEntry, Entry)) { + // Check the stored signature. + if (checkSignature(ModuleEntry->Signature, ExpectedSignature, ErrorStr)) + return OutOfDate; + + Module = ModuleEntry; + updateModuleImports(*ModuleEntry, ImportedBy, ImportLoc); + return AlreadyLoaded; + } } // Allocate a new module.