From 9af107295570abd8a6ba3c23fb4fd146d5f9f448 Mon Sep 17 00:00:00 2001 From: Hamish Knight Date: Sun, 12 Apr 2020 17:22:34 -0700 Subject: [PATCH 01/13] [lldb] Update for removal of SourceFile::addImports Additional modules to implicitly import now need to be provided upfront when constructing the ModuleDecl. To enable this change, split `PerformUserImport` into `GetImplicitImports`, which retrieves the necessary modules to import, and `CacheUserImports`, which deals with caching the imports from the parsed source file such that they're available to future expression evaluations. This commit also renames `PerformAutoImport` to `GetCompileUnitImports` to better match the other two methods. --- lldb/include/lldb/Symbol/SwiftASTContext.h | 43 ++++-- .../Swift/SwiftExpressionParser.cpp | 47 +++--- .../ExpressionParser/Swift/SwiftREPL.cpp | 14 +- lldb/source/Symbol/SwiftASTContext.cpp | 135 +++++++++--------- lldb/source/Target/SwiftLanguageRuntime.cpp | 3 +- lldb/source/Target/Target.cpp | 4 +- 6 files changed, 140 insertions(+), 106 deletions(-) diff --git a/lldb/include/lldb/Symbol/SwiftASTContext.h b/lldb/include/lldb/Symbol/SwiftASTContext.h index 5cd9fccc68cf4..bcdd62a59a5bd 100644 --- a/lldb/include/lldb/Symbol/SwiftASTContext.h +++ b/lldb/include/lldb/Symbol/SwiftASTContext.h @@ -39,6 +39,7 @@ enum class IRGenDebugInfoLevel : unsigned; class CanType; class DependencyTracker; class DWARFImporterDelegate; +struct ImplicitImportInfo; class IRGenOptions; class NominalTypeDecl; class SearchPathOptions; @@ -595,7 +596,13 @@ class SwiftASTContext : public TypeSystemSwift { /// \return the ExtraArgs of the ClangImporterOptions. const std::vector &GetClangArguments(); - swift::ModuleDecl *CreateModule(const SourceModule &module, Status &error); + /// Attempt to create a Swift module, returning \c nullptr and setting + /// \p error if unsuccessful. + /// + /// \param importInfo Information about which modules should be implicitly + /// imported by each file of the module. + swift::ModuleDecl *CreateModule(const SourceModule &module, Status &error, + swift::ImplicitImportInfo importInfo); // This function should only be called when all search paths // for all items in a swift::ASTContext have been setup to @@ -1064,16 +1071,30 @@ class SwiftASTContext : public TypeSystemSwift { void SetCachedType(ConstString mangled, const lldb::TypeSP &type_sp) override; - static bool PerformUserImport(SwiftASTContext &swift_ast_context, - SymbolContext &sc, - ExecutionContextScope &exe_scope, - lldb::StackFrameWP &stack_frame_wp, - swift::SourceFile &source_file, Status &error); - - static bool PerformAutoImport(SwiftASTContext &swift_ast_context, - SymbolContext &sc, - lldb::StackFrameWP &stack_frame_wp, - swift::SourceFile *source_file, Status &error); + /// Retrieves the modules that need to be implicitly imported in a given + /// execution scope. This includes the modules imported by both the compile + /// unit as well as any imports from previous expression evaluations. + static bool + GetImplicitImports(SwiftASTContext &swift_ast_context, SymbolContext &sc, + ExecutionContextScope &exe_scope, + lldb::StackFrameWP &stack_frame_wp, + llvm::SmallVectorImpl &modules, + Status &error); + + /// Cache the user's imports from a SourceFile in a given execution scope such + /// that they are carried over into future expression evaluations. + static bool CacheUserImports(SwiftASTContext &swift_ast_context, + SymbolContext &sc, + ExecutionContextScope &exe_scope, + lldb::StackFrameWP &stack_frame_wp, + swift::SourceFile &source_file, Status &error); + + /// Retrieve the modules imported by the compilation unit. + static bool + GetCompileUnitImports(SwiftASTContext &swift_ast_context, SymbolContext &sc, + lldb::StackFrameWP &stack_frame_wp, + llvm::SmallVectorImpl &modules, + Status &error); protected: /// This map uses the string value of ConstStrings as the key, and the diff --git a/lldb/source/Plugins/ExpressionParser/Swift/SwiftExpressionParser.cpp b/lldb/source/Plugins/ExpressionParser/Swift/SwiftExpressionParser.cpp index 7d7197e3ae316..6e4bf518a79f1 100644 --- a/lldb/source/Plugins/ExpressionParser/Swift/SwiftExpressionParser.cpp +++ b/lldb/source/Plugins/ExpressionParser/Swift/SwiftExpressionParser.cpp @@ -1196,13 +1196,28 @@ static llvm::Expected ParseAndImport( snprintf(expr_name_buf, sizeof(expr_name_buf), "__lldb_expr_%u", options.GetExpressionNumber()); + // Gather the modules that need to be implicitly imported. + // The Swift stdlib needs to be imported before the SwiftLanguageRuntime can + // be used. + Status implicit_import_error; + llvm::SmallVector additional_imports; + if (!SwiftASTContext::GetImplicitImports(*swift_ast_context, sc, exe_scope, + stack_frame_wp, additional_imports, + implicit_import_error)) { + return make_error(llvm::Twine("in implicit-import:\n") + + implicit_import_error.AsCString()); + } + + swift::ImplicitImportInfo importInfo; + importInfo.StdlibKind = swift::ImplicitStdlibKind::Stdlib; + for (auto *module : additional_imports) + importInfo.AdditionalModules.emplace_back(module, /*exported*/ false); + auto module_id = ast_context->getIdentifier(expr_name_buf); - auto &module = *swift::ModuleDecl::create(module_id, *ast_context); - const auto implicit_import_kind = - swift::SourceFile::ImplicitModuleImportKind::Stdlib; + auto &module = *swift::ModuleDecl::create(module_id, *ast_context, + importInfo); swift::SourceFileKind source_file_kind = swift::SourceFileKind::Library; - if (playground || repl) { source_file_kind = swift::SourceFileKind::Main; } @@ -1210,21 +1225,11 @@ static llvm::Expected ParseAndImport( // Create the source file. Note, we disable delayed parsing for the // swift expression parser. swift::SourceFile *source_file = new (*ast_context) swift::SourceFile( - module, source_file_kind, buffer_id, implicit_import_kind, - /*Keep tokens*/ false, /*KeepSyntaxTree*/ false, + module, source_file_kind, buffer_id, /*Keep tokens*/ false, + /*KeepSyntaxTree*/ false, swift::SourceFile::ParsingFlags::DisableDelayedBodies); module.addFile(*source_file); - - // The Swift stdlib needs to be imported before the - // SwiftLanguageRuntime can be used. - Status auto_import_error; - if (!SwiftASTContext::PerformAutoImport(*swift_ast_context, sc, - stack_frame_wp, source_file, - auto_import_error)) - return make_error(llvm::Twine("in auto-import:\n") + - auto_import_error.AsCString()); - // Swift Modules that rely on shared libraries (not frameworks) // don't record the link information in the swiftmodule file, so we // can't really make them work without outside information. @@ -1331,16 +1336,16 @@ static llvm::Expected ParseAndImport( if (swift_ast_context->HasErrors()) return make_error(); - // Do the auto-importing after Name Binding, that's when the Imports - // for the source file are figured out. + // Cache the source file's imports such that they're accessible to future + // expression evaluations. { std::lock_guard global_context_locker( IRExecutionUnit::GetLLVMGlobalContextMutex()); Status auto_import_error; - if (!SwiftASTContext::PerformUserImport(*swift_ast_context, sc, exe_scope, - stack_frame_wp, *source_file, - auto_import_error)) { + if (!SwiftASTContext::CacheUserImports(*swift_ast_context, sc, exe_scope, + stack_frame_wp, *source_file, + auto_import_error)) { return make_error(llvm::Twine("in user-import:\n") + auto_import_error.AsCString()); } diff --git a/lldb/source/Plugins/ExpressionParser/Swift/SwiftREPL.cpp b/lldb/source/Plugins/ExpressionParser/Swift/SwiftREPL.cpp index 2a1b288de9b08..69790b0e8588d 100644 --- a/lldb/source/Plugins/ExpressionParser/Swift/SwiftREPL.cpp +++ b/lldb/source/Plugins/ExpressionParser/Swift/SwiftREPL.cpp @@ -566,18 +566,16 @@ void SwiftREPL::CompleteCode(const std::string ¤t_code, repl_module = swift_ast->GetModule(completion_module_info, error); if (repl_module == nullptr) { - repl_module = swift_ast->CreateModule(completion_module_info, error); - const swift::SourceFile::ImplicitModuleImportKind implicit_import_kind = - swift::SourceFile::ImplicitModuleImportKind::Stdlib; + swift::ImplicitImportInfo importInfo; + importInfo.StdlibKind = swift::ImplicitStdlibKind::Stdlib; + repl_module = swift_ast->CreateModule(completion_module_info, error, + importInfo); llvm::Optional bufferID; swift::SourceFile *repl_source_file = new (*ast) swift::SourceFile(*repl_module, swift::SourceFileKind::REPL, bufferID, - implicit_import_kind, /*Keep tokens*/false); - - // Given this file is empty and only exists to import the standard - // library, we can go ahead and just mark it as having been type checked. - repl_source_file->ASTStage = swift::SourceFile::TypeChecked; + /*Keep tokens*/false); repl_module->addFile(*repl_source_file); + swift::performImportResolution(*repl_source_file); m_completion_module_initialized = true; } if (repl_module) { diff --git a/lldb/source/Symbol/SwiftASTContext.cpp b/lldb/source/Symbol/SwiftASTContext.cpp index 70b94e9459114..2fc7484628424 100644 --- a/lldb/source/Symbol/SwiftASTContext.cpp +++ b/lldb/source/Symbol/SwiftASTContext.cpp @@ -3611,8 +3611,9 @@ SwiftASTContext::GetCachedModule(const SourceModule &module) { return nullptr; } -swift::ModuleDecl *SwiftASTContext::CreateModule(const SourceModule &module, - Status &error) { +swift::ModuleDecl * +SwiftASTContext::CreateModule(const SourceModule &module, Status &error, + swift::ImplicitImportInfo importInfo) { VALID_OR_RETURN(nullptr); if (!module.path.size()) { error.SetErrorStringWithFormat("invalid module name (empty)"); @@ -3633,7 +3634,7 @@ swift::ModuleDecl *SwiftASTContext::CreateModule(const SourceModule &module, swift::Identifier module_id( ast->getIdentifier(module.path.front().GetCString())); - auto *module_decl = swift::ModuleDecl::create(module_id, *ast); + auto *module_decl = swift::ModuleDecl::create(module_id, *ast, importInfo); if (!module_decl) { error.SetErrorStringWithFormat("failed to create module for \"%s\"", module.path.front().GetCString()); @@ -8353,14 +8354,12 @@ static void GetNameFromModule(swift::ModuleDecl *module, std::string &result) { } } -static bool -LoadOneModule(const SourceModule &module, SwiftASTContext &swift_ast_context, - lldb::StackFrameWP &stack_frame_wp, - llvm::SmallVectorImpl - &additional_imports, - Status &error) { +static swift::ModuleDecl *LoadOneModule(const SourceModule &module, + SwiftASTContext &swift_ast_context, + lldb::StackFrameWP &stack_frame_wp, + Status &error) { if (!module.path.size()) - return false; + return nullptr; error.Clear(); ConstString toplevel = module.path.front(); @@ -8399,7 +8398,7 @@ LoadOneModule(const SourceModule &module, SwiftASTContext &swift_ast_context, toplevel.AsCString(), error.AsCString()); if (!swift_module || swift_ast_context.HasFatalErrors()) { - return false; + return nullptr; } } @@ -8410,23 +8409,42 @@ LoadOneModule(const SourceModule &module, SwiftASTContext &swift_ast_context, LOG_PRINTF(LIBLLDB_LOG_EXPRESSIONS, "Imported module %s from {%s}", module.path.front().AsCString(), ss.GetData()); } + return swift_module; +} + +bool SwiftASTContext::GetImplicitImports( + SwiftASTContext &swift_ast_context, SymbolContext &sc, + ExecutionContextScope &exe_scope, lldb::StackFrameWP &stack_frame_wp, + llvm::SmallVectorImpl &modules, Status &error) { + if (!GetCompileUnitImports(swift_ast_context, sc, stack_frame_wp, modules, + error)) { + return false; + } + + auto *persistent_expression_state = + sc.target_sp->GetSwiftPersistentExpressionState(exe_scope); - additional_imports.push_back(swift::SourceFile::ImportedModuleDesc( - std::make_pair(swift::ModuleDecl::AccessPathTy(), swift_module), - swift::SourceFile::ImportOptions())); + // Get the hand-loaded modules from the SwiftPersistentExpressionState. + for (ConstString name : persistent_expression_state->GetHandLoadedModules()) { + SourceModule module_info; + module_info.path.push_back(name); + auto *module = LoadOneModule(module_info, swift_ast_context, stack_frame_wp, + error); + if (!module) + return false; + + modules.push_back(module); + } return true; } -bool SwiftASTContext::PerformUserImport(SwiftASTContext &swift_ast_context, - SymbolContext &sc, - ExecutionContextScope &exe_scope, - lldb::StackFrameWP &stack_frame_wp, - swift::SourceFile &source_file, - Status &error) { +bool SwiftASTContext::CacheUserImports(SwiftASTContext &swift_ast_context, + SymbolContext &sc, + ExecutionContextScope &exe_scope, + lldb::StackFrameWP &stack_frame_wp, + swift::SourceFile &source_file, + Status &error) { llvm::SmallString<1> m_description; - llvm::SmallVector - additional_imports; - llvm::SmallVector parsed_imports; swift::ModuleDecl::ImportFilter import_filter; @@ -8450,7 +8468,7 @@ bool SwiftASTContext::PerformUserImport(SwiftASTContext &swift_ast_context, "Performing auto import on found module: %s.\n", module_name.c_str()); if (!LoadOneModule(module_info, swift_ast_context, stack_frame_wp, - additional_imports, error)) + error)) return false; // How do we tell we are in REPL or playground mode? @@ -8458,54 +8476,43 @@ bool SwiftASTContext::PerformUserImport(SwiftASTContext &swift_ast_context, } } } - // Finally get the hand-loaded modules from the - // SwiftPersistentExpressionState and load them into this context: - for (ConstString name : persistent_expression_state->GetHandLoadedModules()) { - SourceModule module_info; - module_info.path.push_back(name); - if (!LoadOneModule(module_info, swift_ast_context, stack_frame_wp, - additional_imports, error)) - return false; - } - - source_file.addImports(additional_imports); return true; } -bool SwiftASTContext::PerformAutoImport(SwiftASTContext &swift_ast_context, - SymbolContext &sc, - lldb::StackFrameWP &stack_frame_wp, - swift::SourceFile *source_file, - Status &error) { - llvm::SmallVector - additional_imports; - - // Import the Swift standard library and its dependecies. +bool SwiftASTContext::GetCompileUnitImports( + SwiftASTContext &swift_ast_context, SymbolContext &sc, + lldb::StackFrameWP &stack_frame_wp, + llvm::SmallVectorImpl &modules, Status &error) { + // Import the Swift standard library and its dependencies. SourceModule swift_module; swift_module.path.push_back(ConstString("Swift")); - if (!LoadOneModule(swift_module, swift_ast_context, stack_frame_wp, - additional_imports, error)) + auto *stdlib = + LoadOneModule(swift_module, swift_ast_context, stack_frame_wp, error); + if (!stdlib) return false; + modules.push_back(stdlib); + CompileUnit *compile_unit = sc.comp_unit; - if (compile_unit && compile_unit->GetLanguage() == lldb::eLanguageTypeSwift) - for (const SourceModule &module : compile_unit->GetImportedModules()) { - // When building the Swift stdlib with debug info these will - // show up in "Swift.o", but we already imported them and - // manually importing them will fail. - if (module.path.size() && - llvm::StringSwitch(module.path.front().GetStringRef()) - .Cases("Swift", "SwiftShims", "Builtin", true) - .Default(false)) - continue; + if (!compile_unit || compile_unit->GetLanguage() != lldb::eLanguageTypeSwift) + return true; - if (!LoadOneModule(module, swift_ast_context, stack_frame_wp, - additional_imports, error)) - return false; - } - // source_file might be NULL outside of the expression parser, where - // we don't need to notify the source file of additional imports. - if (source_file) - source_file->addImports(additional_imports); + for (const SourceModule &module : compile_unit->GetImportedModules()) { + // When building the Swift stdlib with debug info these will + // show up in "Swift.o", but we already imported them and + // manually importing them will fail. + if (module.path.size() && + llvm::StringSwitch(module.path.front().GetStringRef()) + .Cases("Swift", "SwiftShims", "Builtin", true) + .Default(false)) + continue; + + auto *loaded_module = + LoadOneModule(module, swift_ast_context, stack_frame_wp, error); + if (!loaded_module) + return false; + + modules.push_back(loaded_module); + } return true; } diff --git a/lldb/source/Target/SwiftLanguageRuntime.cpp b/lldb/source/Target/SwiftLanguageRuntime.cpp index a2f4cb4818379..d9e59f809ff37 100644 --- a/lldb/source/Target/SwiftLanguageRuntime.cpp +++ b/lldb/source/Target/SwiftLanguageRuntime.cpp @@ -1198,7 +1198,8 @@ void SwiftLanguageRuntime::RegisterGlobalError(Target &target, ConstString name, Status module_creation_error; swift::ModuleDecl *module_decl = - ast_context->CreateModule(module_info, module_creation_error); + ast_context->CreateModule(module_info, module_creation_error, + /*importInfo*/ {}); if (module_creation_error.Success() && module_decl) { const bool is_static = false; diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp index 035f15b3a01f0..ada11357234d8 100644 --- a/lldb/source/Target/Target.cpp +++ b/lldb/source/Target/Target.cpp @@ -2499,7 +2499,9 @@ SwiftASTContextReader Target::GetScratchSwiftASTContext( !swift_ast_ctx->HasFatalErrors()) { StackFrameWP frame_wp(frame_sp); SymbolContext sc = frame_sp->GetSymbolContext(lldb::eSymbolContextEverything); - swift_ast_ctx->PerformAutoImport(*swift_ast_ctx, sc, frame_wp, nullptr, error); + llvm::SmallVector modules; + swift_ast_ctx->GetCompileUnitImports(*swift_ast_ctx, sc, frame_wp, modules, + error); } return SwiftASTContextReader(GetSwiftScratchContextLock(), swift_ast_ctx); From f53d12b2b0b7ea677ff6aeedcb303e8cd8ec2fbc Mon Sep 17 00:00:00 2001 From: Hamish Knight Date: Tue, 14 Apr 2020 16:26:28 -0700 Subject: [PATCH 02/13] [lldb] Resolve imports before manipulating AST Imports need to be resolved before the SwiftASTManipulator is run, as it can query interface types. --- .../ExpressionParser/Swift/SwiftExpressionParser.cpp | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/lldb/source/Plugins/ExpressionParser/Swift/SwiftExpressionParser.cpp b/lldb/source/Plugins/ExpressionParser/Swift/SwiftExpressionParser.cpp index 6e4bf518a79f1..03da08b60d391 100644 --- a/lldb/source/Plugins/ExpressionParser/Swift/SwiftExpressionParser.cpp +++ b/lldb/source/Plugins/ExpressionParser/Swift/SwiftExpressionParser.cpp @@ -1280,6 +1280,13 @@ static llvm::Expected ParseAndImport( // inserting them in. swift_ast_context->AddDebuggerClient(external_lookup); + if (swift_ast_context->HasErrors()) + return make_error(); + + // Resolve the file's imports, including the implicit ones returned from + // GetImplicitImports. + swift::performImportResolution(*source_file); + if (swift_ast_context->HasErrors()) return make_error(); @@ -1331,11 +1338,6 @@ static llvm::Expected ParseAndImport( stack_frame_sp.reset(); } - swift::performImportResolution(*source_file); - - if (swift_ast_context->HasErrors()) - return make_error(); - // Cache the source file's imports such that they're accessible to future // expression evaluations. { From 05f5e2a8024683b27af61a2b878af097579f1d8a Mon Sep 17 00:00:00 2001 From: Amara Emerson Date: Mon, 24 Feb 2020 14:27:32 -0800 Subject: [PATCH 03/13] [AArch64][GlobalISel] Fixup <32b heterogeneous regbanks of G_PHIs just before selection. Since all types <32b on gpr end up being assigned gpr32 regclasses, we can end up with PHIs here which try to select between a gpr32 and an fpr16. Ideally RBS shouldn't be selecting heterogenous regbanks for operands if possible, but we still need to be able to deal with it here. To fix this, if we have a gpr-bank operand < 32b in size and at least one other operand is on the fpr bank, then we add cross-bank copies to homogenize the operand banks. For simplicity the bank that we choose to settle on is whatever bank the def operand has. For example: %endbb: %dst:gpr(s16) = G_PHI %in1:gpr(s16), %bb1, %in2:fpr(s16), %bb2 => %bb2: ... %in2_copy:gpr(s16) = COPY %in2:fpr(s16) ... %endbb: %dst:gpr(s16) = G_PHI %in1:gpr(s16), %bb1, %in2_copy:gpr(s16), %bb2 Differential Revision: https://reviews.llvm.org/D75086 (cherry picked from commit 65f99b5383ff3293881f59dd64cfb596c3d03aa4) --- .../AArch64/AArch64InstructionSelector.cpp | 93 +++++++++++++++ .../GlobalISel/preselect-process-phis.mir | 110 ++++++++++++++++++ 2 files changed, 203 insertions(+) create mode 100644 llvm/test/CodeGen/AArch64/GlobalISel/preselect-process-phis.mir diff --git a/llvm/lib/Target/AArch64/AArch64InstructionSelector.cpp b/llvm/lib/Target/AArch64/AArch64InstructionSelector.cpp index 193136fe53d8c..6b109ce696de6 100644 --- a/llvm/lib/Target/AArch64/AArch64InstructionSelector.cpp +++ b/llvm/lib/Target/AArch64/AArch64InstructionSelector.cpp @@ -63,6 +63,7 @@ class AArch64InstructionSelector : public InstructionSelector { // cache it here for each run of the selector. ProduceNonFlagSettingCondBr = !MF.getFunction().hasFnAttribute(Attribute::SpeculativeLoadHardening); + processPHIs(MF); } private: @@ -77,6 +78,9 @@ class AArch64InstructionSelector : public InstructionSelector { // An early selection function that runs before the selectImpl() call. bool earlySelect(MachineInstr &I) const; + // Do some preprocessing of G_PHIs before we begin selection. + void processPHIs(MachineFunction &MF); + bool earlySelectSHL(MachineInstr &I, MachineRegisterInfo &MRI) const; /// Eliminate same-sized cross-bank copies into stores before selectImpl(). @@ -4755,6 +4759,95 @@ bool AArch64InstructionSelector::isDef32(const MachineInstr &MI) const { } } + +// Perform fixups on the given PHI instruction's operands to force them all +// to be the same as the destination regbank. +static void fixupPHIOpBanks(MachineInstr &MI, MachineRegisterInfo &MRI, + const AArch64RegisterBankInfo &RBI) { + assert(MI.getOpcode() == TargetOpcode::G_PHI && "Expected a G_PHI"); + Register DstReg = MI.getOperand(0).getReg(); + const RegisterBank *DstRB = MRI.getRegBankOrNull(DstReg); + assert(DstRB && "Expected PHI dst to have regbank assigned"); + MachineIRBuilder MIB(MI); + + // Go through each operand and ensure it has the same regbank. + for (unsigned OpIdx = 1; OpIdx < MI.getNumOperands(); ++OpIdx) { + MachineOperand &MO = MI.getOperand(OpIdx); + if (!MO.isReg()) + continue; + Register OpReg = MO.getReg(); + const RegisterBank *RB = MRI.getRegBankOrNull(OpReg); + if (RB != DstRB) { + // Insert a cross-bank copy. + auto *OpDef = MRI.getVRegDef(OpReg); + const LLT &Ty = MRI.getType(OpReg); + MIB.setInsertPt(*OpDef->getParent(), std::next(OpDef->getIterator())); + auto Copy = MIB.buildCopy(Ty, OpReg); + MRI.setRegBank(Copy.getReg(0), *DstRB); + MO.setReg(Copy.getReg(0)); + } + } +} + +void AArch64InstructionSelector::processPHIs(MachineFunction &MF) { + // We're looking for PHIs, build a list so we don't invalidate iterators. + MachineRegisterInfo &MRI = MF.getRegInfo(); + SmallVector Phis; + for (auto &BB : MF) { + for (auto &MI : BB) { + if (MI.getOpcode() == TargetOpcode::G_PHI) + Phis.emplace_back(&MI); + } + } + + for (auto *MI : Phis) { + // We need to do some work here if the operand types are < 16 bit and they + // are split across fpr/gpr banks. Since all types <32b on gpr + // end up being assigned gpr32 regclasses, we can end up with PHIs here + // which try to select between a gpr32 and an fpr16. Ideally RBS shouldn't + // be selecting heterogenous regbanks for operands if possible, but we + // still need to be able to deal with it here. + // + // To fix this, if we have a gpr-bank operand < 32b in size and at least + // one other operand is on the fpr bank, then we add cross-bank copies + // to homogenize the operand banks. For simplicity the bank that we choose + // to settle on is whatever bank the def operand has. For example: + // + // %endbb: + // %dst:gpr(s16) = G_PHI %in1:gpr(s16), %bb1, %in2:fpr(s16), %bb2 + // => + // %bb2: + // ... + // %in2_copy:gpr(s16) = COPY %in2:fpr(s16) + // ... + // %endbb: + // %dst:gpr(s16) = G_PHI %in1:gpr(s16), %bb1, %in2_copy:gpr(s16), %bb2 + bool HasGPROp = false, HasFPROp = false; + for (unsigned OpIdx = 1; OpIdx < MI->getNumOperands(); ++OpIdx) { + const auto &MO = MI->getOperand(OpIdx); + if (!MO.isReg()) + continue; + const LLT &Ty = MRI.getType(MO.getReg()); + if (!Ty.isValid() || !Ty.isScalar()) + break; + if (Ty.getSizeInBits() >= 32) + break; + const RegisterBank *RB = MRI.getRegBankOrNull(MO.getReg()); + // If for some reason we don't have a regbank yet. Don't try anything. + if (!RB) + break; + + if (RB->getID() == AArch64::GPRRegBankID) + HasGPROp = true; + else + HasFPROp = true; + } + // We have heterogenous regbanks, need to fixup. + if (HasGPROp && HasFPROp) + fixupPHIOpBanks(*MI, MRI, RBI); + } +} + namespace llvm { InstructionSelector * createAArch64InstructionSelector(const AArch64TargetMachine &TM, diff --git a/llvm/test/CodeGen/AArch64/GlobalISel/preselect-process-phis.mir b/llvm/test/CodeGen/AArch64/GlobalISel/preselect-process-phis.mir new file mode 100644 index 0000000000000..1b7c074018996 --- /dev/null +++ b/llvm/test/CodeGen/AArch64/GlobalISel/preselect-process-phis.mir @@ -0,0 +1,110 @@ +# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py +# RUN: llc -verify-machineinstrs -mtriple aarch64--- -run-pass=instruction-select -global-isel %s -o - | FileCheck %s +--- +name: test_loop_phi_fpr_to_gpr +alignment: 4 +legalized: true +regBankSelected: true +selected: false +failedISel: false +tracksRegLiveness: true +liveins: [] +machineFunctionInfo: {} +body: | + ; CHECK-LABEL: name: test_loop_phi_fpr_to_gpr + ; CHECK: bb.0: + ; CHECK: successors: %bb.1(0x80000000) + ; CHECK: [[DEF:%[0-9]+]]:gpr32 = IMPLICIT_DEF + ; CHECK: [[DEF1:%[0-9]+]]:gpr64common = IMPLICIT_DEF + ; CHECK: [[MOVi32imm:%[0-9]+]]:gpr32 = MOVi32imm 2143289344 + ; CHECK: [[COPY:%[0-9]+]]:fpr32 = COPY [[MOVi32imm]] + ; CHECK: bb.1: + ; CHECK: successors: %bb.2(0x80000000) + ; CHECK: [[DEF2:%[0-9]+]]:gpr32 = IMPLICIT_DEF + ; CHECK: $wzr = ANDSWri [[DEF]], 0, implicit-def $nzcv + ; CHECK: [[CSELWr:%[0-9]+]]:gpr32 = CSELWr [[DEF2]], [[DEF2]], 1, implicit $nzcv + ; CHECK: bb.2: + ; CHECK: successors: %bb.2(0x80000000) + ; CHECK: [[PHI:%[0-9]+]]:gpr32 = PHI [[CSELWr]], %bb.1, %8, %bb.2 + ; CHECK: [[FCVTHSr:%[0-9]+]]:fpr16 = FCVTHSr [[COPY]] + ; CHECK: [[SUBREG_TO_REG:%[0-9]+]]:fpr32 = SUBREG_TO_REG 0, [[FCVTHSr]], %subreg.hsub + ; CHECK: [[COPY1:%[0-9]+]]:gpr32all = COPY [[SUBREG_TO_REG]] + ; CHECK: STRHHui [[PHI]], [[DEF1]], 0 :: (store 2 into `half* undef`) + ; CHECK: B %bb.2 + bb.0: + successors: %bb.1(0x80000000) + + %0:gpr(s1) = G_IMPLICIT_DEF + %4:gpr(p0) = G_IMPLICIT_DEF + %8:fpr(s32) = G_FCONSTANT float 0x7FF8000000000000 + + bb.1: + successors: %bb.2(0x80000000) + + %6:gpr(s32) = G_IMPLICIT_DEF + %7:gpr(s32) = G_SELECT %0(s1), %6, %6 + %1:gpr(s16) = G_TRUNC %7(s32) + + bb.2: + successors: %bb.2(0x80000000) + + %3:gpr(s16) = G_PHI %1(s16), %bb.1, %5(s16), %bb.2 + %5:fpr(s16) = G_FPTRUNC %8(s32) + G_STORE %3(s16), %4(p0) :: (store 2 into `half* undef`) + G_BR %bb.2 + +... +--- +name: test_loop_phi_gpr_to_fpr +alignment: 4 +legalized: true +regBankSelected: true +selected: false +failedISel: false +tracksRegLiveness: true +liveins: [] +machineFunctionInfo: {} +body: | + ; CHECK-LABEL: name: test_loop_phi_gpr_to_fpr + ; CHECK: bb.0: + ; CHECK: successors: %bb.1(0x80000000) + ; CHECK: [[DEF:%[0-9]+]]:gpr32 = IMPLICIT_DEF + ; CHECK: [[DEF1:%[0-9]+]]:gpr64common = IMPLICIT_DEF + ; CHECK: [[MOVi32imm:%[0-9]+]]:gpr32 = MOVi32imm 2143289344 + ; CHECK: [[COPY:%[0-9]+]]:fpr32 = COPY [[MOVi32imm]] + ; CHECK: bb.1: + ; CHECK: successors: %bb.2(0x80000000) + ; CHECK: [[DEF2:%[0-9]+]]:gpr32 = IMPLICIT_DEF + ; CHECK: $wzr = ANDSWri [[DEF]], 0, implicit-def $nzcv + ; CHECK: [[CSELWr:%[0-9]+]]:gpr32 = CSELWr [[DEF2]], [[DEF2]], 1, implicit $nzcv + ; CHECK: [[COPY1:%[0-9]+]]:fpr32 = COPY [[CSELWr]] + ; CHECK: [[COPY2:%[0-9]+]]:fpr16 = COPY [[COPY1]].hsub + ; CHECK: bb.2: + ; CHECK: successors: %bb.2(0x80000000) + ; CHECK: [[PHI:%[0-9]+]]:fpr16 = PHI %7, %bb.2, [[COPY2]], %bb.1 + ; CHECK: [[FCVTHSr:%[0-9]+]]:fpr16 = FCVTHSr [[COPY]] + ; CHECK: STRHui [[PHI]], [[DEF1]], 0 :: (store 2 into `half* undef`) + ; CHECK: B %bb.2 + bb.0: + successors: %bb.1(0x80000000) + + %0:gpr(s1) = G_IMPLICIT_DEF + %4:gpr(p0) = G_IMPLICIT_DEF + %8:fpr(s32) = G_FCONSTANT float 0x7FF8000000000000 + + bb.1: + successors: %bb.2(0x80000000) + + %6:gpr(s32) = G_IMPLICIT_DEF + %7:gpr(s32) = G_SELECT %0(s1), %6, %6 + %1:gpr(s16) = G_TRUNC %7(s32) + + bb.2: + successors: %bb.2(0x80000000) + + %3:fpr(s16) = G_PHI %5(s16), %bb.2, %1(s16), %bb.1 + %5:fpr(s16) = G_FPTRUNC %8(s32) + G_STORE %3(s16), %4(p0) :: (store 2 into `half* undef`) + G_BR %bb.2 + +... From 750e49cf5a6f03cb5aff0cebe1612c7894d5b56e Mon Sep 17 00:00:00 2001 From: Kadir Cetinkaya Date: Wed, 8 Jan 2020 11:21:21 +0100 Subject: [PATCH 04/13] Revert "[InstCombine] fold zext of masked bit set/clear" This reverts commit a041c4ec6f7aa659b235cb67e9231a05e0a33b7d. This looks like a non-trivial change and there has been no code reviews (at least there were no phabricator revisions attached to the commit description). It is also causing a regression in one of our downstream integration tests, we haven't been able to come up with a minimal reproducer yet. (cherry picked from commit b212eb7159b40c98b3c40619b82b996fb903282b) --- .../InstCombine/InstCombineCasts.cpp | 20 +----- llvm/test/Transforms/InstCombine/zext.ll | 65 ++++++++----------- 2 files changed, 31 insertions(+), 54 deletions(-) diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp index b9be41840ffaf..3ba56bbe53e06 100644 --- a/llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp +++ b/llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp @@ -922,24 +922,10 @@ Instruction *InstCombiner::transformZExtICmp(ICmpInst *Cmp, ZExtInst &Zext, } } + // icmp ne A, B is equal to xor A, B when A and B only really have one bit. + // It is also profitable to transform icmp eq into not(xor(A, B)) because that + // may lead to additional simplifications. if (Cmp->isEquality() && Zext.getType() == Cmp->getOperand(0)->getType()) { - // Test if a bit is clear/set using a shifted-one mask: - // zext (icmp eq (and X, (1 << ShAmt)), 0) --> and (lshr (not X), ShAmt), 1 - // zext (icmp ne (and X, (1 << ShAmt)), 0) --> and (lshr X, ShAmt), 1 - Value *X, *ShAmt; - if (Cmp->hasOneUse() && match(Cmp->getOperand(1), m_ZeroInt()) && - match(Cmp->getOperand(0), - m_OneUse(m_c_And(m_Shl(m_One(), m_Value(ShAmt)), m_Value(X))))) { - if (Cmp->getPredicate() == ICmpInst::ICMP_EQ) - X = Builder.CreateNot(X); - Value *Lshr = Builder.CreateLShr(X, ShAmt); - Value *And1 = Builder.CreateAnd(Lshr, ConstantInt::get(X->getType(), 1)); - return replaceInstUsesWith(Zext, And1); - } - - // icmp ne A, B is equal to xor A, B when A and B only really have one bit. - // It is also profitable to transform icmp eq into not(xor(A, B)) because - // that may lead to additional simplifications. if (IntegerType *ITy = dyn_cast(Zext.getType())) { Value *LHS = Cmp->getOperand(0); Value *RHS = Cmp->getOperand(1); diff --git a/llvm/test/Transforms/InstCombine/zext.ll b/llvm/test/Transforms/InstCombine/zext.ll index 9351f5cea4dd1..1dbb9ffd7e083 100644 --- a/llvm/test/Transforms/InstCombine/zext.ll +++ b/llvm/test/Transforms/InstCombine/zext.ll @@ -177,9 +177,11 @@ declare void @use32(i32) define i32 @masked_bit_set(i32 %x, i32 %y) { ; CHECK-LABEL: @masked_bit_set( -; CHECK-NEXT: [[TMP1:%.*]] = lshr i32 [[X:%.*]], [[Y:%.*]] -; CHECK-NEXT: [[TMP2:%.*]] = and i32 [[TMP1]], 1 -; CHECK-NEXT: ret i32 [[TMP2]] +; CHECK-NEXT: [[SH1:%.*]] = shl i32 1, [[Y:%.*]] +; CHECK-NEXT: [[AND:%.*]] = and i32 [[SH1]], [[X:%.*]] +; CHECK-NEXT: [[CMP:%.*]] = icmp ne i32 [[AND]], 0 +; CHECK-NEXT: [[R:%.*]] = zext i1 [[CMP]] to i32 +; CHECK-NEXT: ret i32 [[R]] ; %sh1 = shl i32 1, %y %and = and i32 %sh1, %x @@ -190,10 +192,11 @@ define i32 @masked_bit_set(i32 %x, i32 %y) { define <2 x i32> @masked_bit_clear(<2 x i32> %x, <2 x i32> %y) { ; CHECK-LABEL: @masked_bit_clear( -; CHECK-NEXT: [[TMP1:%.*]] = xor <2 x i32> [[X:%.*]], -; CHECK-NEXT: [[TMP2:%.*]] = lshr <2 x i32> [[TMP1]], [[Y:%.*]] -; CHECK-NEXT: [[TMP3:%.*]] = and <2 x i32> [[TMP2]], -; CHECK-NEXT: ret <2 x i32> [[TMP3]] +; CHECK-NEXT: [[SH1:%.*]] = shl <2 x i32> , [[Y:%.*]] +; CHECK-NEXT: [[AND:%.*]] = and <2 x i32> [[SH1]], [[X:%.*]] +; CHECK-NEXT: [[CMP:%.*]] = icmp eq <2 x i32> [[AND]], zeroinitializer +; CHECK-NEXT: [[R:%.*]] = zext <2 x i1> [[CMP]] to <2 x i32> +; CHECK-NEXT: ret <2 x i32> [[R]] ; %sh1 = shl <2 x i32> , %y %and = and <2 x i32> %sh1, %x @@ -205,9 +208,11 @@ define <2 x i32> @masked_bit_clear(<2 x i32> %x, <2 x i32> %y) { define <2 x i32> @masked_bit_set_commute(<2 x i32> %px, <2 x i32> %y) { ; CHECK-LABEL: @masked_bit_set_commute( ; CHECK-NEXT: [[X:%.*]] = srem <2 x i32> , [[PX:%.*]] -; CHECK-NEXT: [[TMP1:%.*]] = lshr <2 x i32> [[X]], [[Y:%.*]] -; CHECK-NEXT: [[TMP2:%.*]] = and <2 x i32> [[TMP1]], -; CHECK-NEXT: ret <2 x i32> [[TMP2]] +; CHECK-NEXT: [[SH1:%.*]] = shl <2 x i32> , [[Y:%.*]] +; CHECK-NEXT: [[AND:%.*]] = and <2 x i32> [[X]], [[SH1]] +; CHECK-NEXT: [[CMP:%.*]] = icmp ne <2 x i32> [[AND]], zeroinitializer +; CHECK-NEXT: [[R:%.*]] = zext <2 x i1> [[CMP]] to <2 x i32> +; CHECK-NEXT: ret <2 x i32> [[R]] ; %x = srem <2 x i32> , %px ; thwart complexity-based canonicalization %sh1 = shl <2 x i32> , %y @@ -220,10 +225,11 @@ define <2 x i32> @masked_bit_set_commute(<2 x i32> %px, <2 x i32> %y) { define i32 @masked_bit_clear_commute(i32 %px, i32 %y) { ; CHECK-LABEL: @masked_bit_clear_commute( ; CHECK-NEXT: [[X:%.*]] = srem i32 42, [[PX:%.*]] -; CHECK-NEXT: [[TMP1:%.*]] = xor i32 [[X]], -1 -; CHECK-NEXT: [[TMP2:%.*]] = lshr i32 [[TMP1]], [[Y:%.*]] -; CHECK-NEXT: [[TMP3:%.*]] = and i32 [[TMP2]], 1 -; CHECK-NEXT: ret i32 [[TMP3]] +; CHECK-NEXT: [[SH1:%.*]] = shl i32 1, [[Y:%.*]] +; CHECK-NEXT: [[AND:%.*]] = and i32 [[X]], [[SH1]] +; CHECK-NEXT: [[CMP:%.*]] = icmp eq i32 [[AND]], 0 +; CHECK-NEXT: [[R:%.*]] = zext i1 [[CMP]] to i32 +; CHECK-NEXT: ret i32 [[R]] ; %x = srem i32 42, %px ; thwart complexity-based canonicalization %sh1 = shl i32 1, %y @@ -237,9 +243,10 @@ define i32 @masked_bit_set_use1(i32 %x, i32 %y) { ; CHECK-LABEL: @masked_bit_set_use1( ; CHECK-NEXT: [[SH1:%.*]] = shl i32 1, [[Y:%.*]] ; CHECK-NEXT: call void @use32(i32 [[SH1]]) -; CHECK-NEXT: [[TMP1:%.*]] = lshr i32 [[X:%.*]], [[Y]] -; CHECK-NEXT: [[TMP2:%.*]] = and i32 [[TMP1]], 1 -; CHECK-NEXT: ret i32 [[TMP2]] +; CHECK-NEXT: [[AND:%.*]] = and i32 [[SH1]], [[X:%.*]] +; CHECK-NEXT: [[CMP:%.*]] = icmp ne i32 [[AND]], 0 +; CHECK-NEXT: [[R:%.*]] = zext i1 [[CMP]] to i32 +; CHECK-NEXT: ret i32 [[R]] ; %sh1 = shl i32 1, %y call void @use32(i32 %sh1) @@ -249,8 +256,6 @@ define i32 @masked_bit_set_use1(i32 %x, i32 %y) { ret i32 %r } -; Negative test - define i32 @masked_bit_set_use2(i32 %x, i32 %y) { ; CHECK-LABEL: @masked_bit_set_use2( ; CHECK-NEXT: [[SH1:%.*]] = shl i32 1, [[Y:%.*]] @@ -268,8 +273,6 @@ define i32 @masked_bit_set_use2(i32 %x, i32 %y) { ret i32 %r } -; Negative test - define i32 @masked_bit_set_use3(i32 %x, i32 %y) { ; CHECK-LABEL: @masked_bit_set_use3( ; CHECK-NEXT: [[SH1:%.*]] = shl i32 1, [[Y:%.*]] @@ -291,10 +294,10 @@ define i32 @masked_bit_clear_use1(i32 %x, i32 %y) { ; CHECK-LABEL: @masked_bit_clear_use1( ; CHECK-NEXT: [[SH1:%.*]] = shl i32 1, [[Y:%.*]] ; CHECK-NEXT: call void @use32(i32 [[SH1]]) -; CHECK-NEXT: [[TMP1:%.*]] = xor i32 [[X:%.*]], -1 -; CHECK-NEXT: [[TMP2:%.*]] = lshr i32 [[TMP1]], [[Y]] -; CHECK-NEXT: [[TMP3:%.*]] = and i32 [[TMP2]], 1 -; CHECK-NEXT: ret i32 [[TMP3]] +; CHECK-NEXT: [[AND:%.*]] = and i32 [[SH1]], [[X:%.*]] +; CHECK-NEXT: [[CMP:%.*]] = icmp eq i32 [[AND]], 0 +; CHECK-NEXT: [[R:%.*]] = zext i1 [[CMP]] to i32 +; CHECK-NEXT: ret i32 [[R]] ; %sh1 = shl i32 1, %y call void @use32(i32 %sh1) @@ -304,8 +307,6 @@ define i32 @masked_bit_clear_use1(i32 %x, i32 %y) { ret i32 %r } -; Negative test - define i32 @masked_bit_clear_use2(i32 %x, i32 %y) { ; CHECK-LABEL: @masked_bit_clear_use2( ; CHECK-NEXT: [[SH1:%.*]] = shl i32 1, [[Y:%.*]] @@ -323,8 +324,6 @@ define i32 @masked_bit_clear_use2(i32 %x, i32 %y) { ret i32 %r } -; Negative test - define i32 @masked_bit_clear_use3(i32 %x, i32 %y) { ; CHECK-LABEL: @masked_bit_clear_use3( ; CHECK-NEXT: [[SH1:%.*]] = shl i32 1, [[Y:%.*]] @@ -342,8 +341,6 @@ define i32 @masked_bit_clear_use3(i32 %x, i32 %y) { ret i32 %r } -; Negative test - define i32 @masked_bits_set(i32 %x, i32 %y) { ; CHECK-LABEL: @masked_bits_set( ; CHECK-NEXT: [[SH1:%.*]] = shl i32 3, [[Y:%.*]] @@ -359,8 +356,6 @@ define i32 @masked_bits_set(i32 %x, i32 %y) { ret i32 %r } -; Negative test - define i32 @div_bit_set(i32 %x, i32 %y) { ; CHECK-LABEL: @div_bit_set( ; CHECK-NEXT: [[SH1:%.*]] = shl i32 1, [[Y:%.*]] @@ -376,8 +371,6 @@ define i32 @div_bit_set(i32 %x, i32 %y) { ret i32 %r } -; Negative test - define i32 @masked_bit_set_nonzero_cmp(i32 %x, i32 %y) { ; CHECK-LABEL: @masked_bit_set_nonzero_cmp( ; CHECK-NEXT: [[SH1:%.*]] = shl i32 1, [[Y:%.*]] @@ -393,8 +386,6 @@ define i32 @masked_bit_set_nonzero_cmp(i32 %x, i32 %y) { ret i32 %r } -; Negative test - define i32 @masked_bit_wrong_pred(i32 %x, i32 %y) { ; CHECK-LABEL: @masked_bit_wrong_pred( ; CHECK-NEXT: [[SH1:%.*]] = shl i32 1, [[Y:%.*]] From 87b6ab2eaffe5bd67637dc3f03f97460c41249e8 Mon Sep 17 00:00:00 2001 From: Jonas Devlieghere Date: Tue, 21 Apr 2020 08:25:44 -0700 Subject: [PATCH 05/13] [lldb/Test] Decode stdout and stderr in case it contains Unicode. Lit's to_string will just return the string when it's a `str` instance, which in Python 2 can still contain UTF-8 characters. Differential revision: https://reviews.llvm.org/D76955 (cherry picked from commit 2de52422acf04662b45599f77c14ce1b2cec2b81) --- lldb/test/API/lldbtest.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lldb/test/API/lldbtest.py b/lldb/test/API/lldbtest.py index 94c0588fd9e63..7af4ea7f5b394 100644 --- a/lldb/test/API/lldbtest.py +++ b/lldb/test/API/lldbtest.py @@ -99,6 +99,11 @@ def execute(self, test, litConfig): timeoutInfo = 'Reached timeout of {} seconds'.format( litConfig.maxIndividualTestTime) + if sys.version_info.major == 2: + # In Python 2, string objects can contain Unicode characters. + out = out.decode('utf-8') + err = err.decode('utf-8') + output = """Script:\n--\n%s\n--\nExit Code: %d\n""" % ( ' '.join(cmd), exitCode) if timeoutInfo is not None: From 07e680a59f96fc792583eb567ef1f48c4b66194b Mon Sep 17 00:00:00 2001 From: Jonas Devlieghere Date: Thu, 26 Mar 2020 08:45:08 -0700 Subject: [PATCH 06/13] [lldb/Test] XFAIL TestDataFormatterObjCNSError except for gmodules. --- .../data-formatter-objc/TestDataFormatterObjCNSError.py | 1 + 1 file changed, 1 insertion(+) diff --git a/lldb/test/API/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCNSError.py b/lldb/test/API/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCNSError.py index df380aefd88f4..b5e1f149354fd 100644 --- a/lldb/test/API/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCNSError.py +++ b/lldb/test/API/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCNSError.py @@ -15,6 +15,7 @@ class ObjCDataFormatterNSError(ObjCDataFormatterTestCase): @skipUnlessDarwin + @expectedFailureAll(debug_info=["dwarf", "dsym", "dwo"], bugnumber="rdar://25587546") def test_nserror_with_run_command(self): """Test formatters for NSError.""" self.appkit_tester_impl(self.nserror_data_formatter_commands) From 715aa78694eb1d593b60831146143b39f9fa0034 Mon Sep 17 00:00:00 2001 From: Jonas Devlieghere Date: Fri, 27 Mar 2020 15:02:24 -0700 Subject: [PATCH 07/13] [VirtualFileSystem] Support directory entries in the YAMLVFSWriter The current implementation of the JSONWriter does not support writing out directory entries. Earlier today I added a unit test to illustrate the problem. When an entry is added to the YAMLVFSWriter and the path is a directory, it will incorrectly emit the directory as a file, and any files inside that directory will not be found by the VFS. It's possible to partially work around the issue by only adding "leaf nodes" (files) to the YAMLVFSWriter. However, this doesn't work for representing empty directories. This is a problem for clients of the VFS that want to iterate over a directory. The directory not being there is not the same as the directory being empty. This is not just a hypothetical problem. The FileCollector for example does not differentiate between file and directory paths. I temporarily worked around the issue for LLDB by ignoring directories, but I suspect this will prove problematic sooner rather than later. This patch fixes the issue by extending the JSONWriter to support writing out directory entries. We store whether an entry should be emitted as a file or directory. Differential revision: https://reviews.llvm.org/D76670 (cherry picked from commit 3ef33e69de085cb6bc028b4fc4dd39087631ac12) --- llvm/include/llvm/Support/VirtualFileSystem.h | 9 +++- llvm/lib/Support/VirtualFileSystem.cpp | 34 ++++++++++--- .../Support/VirtualFileSystemTest.cpp | 50 +++++++++++++++++++ 3 files changed, 83 insertions(+), 10 deletions(-) diff --git a/llvm/include/llvm/Support/VirtualFileSystem.h b/llvm/include/llvm/Support/VirtualFileSystem.h index e45e6e7567863..257f7586953ef 100644 --- a/llvm/include/llvm/Support/VirtualFileSystem.h +++ b/llvm/include/llvm/Support/VirtualFileSystem.h @@ -506,10 +506,12 @@ getVFSFromYAML(std::unique_ptr Buffer, struct YAMLVFSEntry { template - YAMLVFSEntry(T1 &&VPath, T2 &&RPath) - : VPath(std::forward(VPath)), RPath(std::forward(RPath)) {} + YAMLVFSEntry(T1 &&VPath, T2 &&RPath, bool IsDirectory = false) + : VPath(std::forward(VPath)), RPath(std::forward(RPath)), + IsDirectory(IsDirectory) {} std::string VPath; std::string RPath; + bool IsDirectory = false; }; class VFSFromYamlDirIterImpl; @@ -781,10 +783,13 @@ class YAMLVFSWriter { Optional UseExternalNames; std::string OverlayDir; + void addEntry(StringRef VirtualPath, StringRef RealPath, bool IsDirectory); + public: YAMLVFSWriter() = default; void addFileMapping(StringRef VirtualPath, StringRef RealPath); + void addDirectoryMapping(StringRef VirtualPath, StringRef RealPath); void setCaseSensitivity(bool CaseSensitive) { IsCaseSensitive = CaseSensitive; diff --git a/llvm/lib/Support/VirtualFileSystem.cpp b/llvm/lib/Support/VirtualFileSystem.cpp index edd4234fe5016..2b899e90e0ae7 100644 --- a/llvm/lib/Support/VirtualFileSystem.cpp +++ b/llvm/lib/Support/VirtualFileSystem.cpp @@ -1894,11 +1894,21 @@ UniqueID vfs::getNextVirtualUniqueID() { return UniqueID(std::numeric_limits::max(), ID); } -void YAMLVFSWriter::addFileMapping(StringRef VirtualPath, StringRef RealPath) { +void YAMLVFSWriter::addEntry(StringRef VirtualPath, StringRef RealPath, + bool IsDirectory) { assert(sys::path::is_absolute(VirtualPath) && "virtual path not absolute"); assert(sys::path::is_absolute(RealPath) && "real path not absolute"); assert(!pathHasTraversal(VirtualPath) && "path traversal is not supported"); - Mappings.emplace_back(VirtualPath, RealPath); + Mappings.emplace_back(VirtualPath, RealPath, IsDirectory); +} + +void YAMLVFSWriter::addFileMapping(StringRef VirtualPath, StringRef RealPath) { + addEntry(VirtualPath, RealPath, /*IsDirectory=*/false); +} + +void YAMLVFSWriter::addDirectoryMapping(StringRef VirtualPath, + StringRef RealPath) { + addEntry(VirtualPath, RealPath, /*IsDirectory=*/true); } namespace { @@ -1999,7 +2009,10 @@ void JSONWriter::write(ArrayRef Entries, if (!Entries.empty()) { const YAMLVFSEntry &Entry = Entries.front(); - startDirectory(path::parent_path(Entry.VPath)); + bool first_entry_is_directory = Entry.IsDirectory; + StringRef Dir = + first_entry_is_directory ? Entry.VPath : path::parent_path(Entry.VPath); + startDirectory(Dir); StringRef RPath = Entry.RPath; if (UseOverlayRelative) { @@ -2009,13 +2022,18 @@ void JSONWriter::write(ArrayRef Entries, RPath = RPath.slice(OverlayDirLen, RPath.size()); } - writeEntry(path::filename(Entry.VPath), RPath); + if (!first_entry_is_directory) + writeEntry(path::filename(Entry.VPath), RPath); for (const auto &Entry : Entries.slice(1)) { - StringRef Dir = path::parent_path(Entry.VPath); - if (Dir == DirStack.back()) - OS << ",\n"; - else { + StringRef Dir = + Entry.IsDirectory ? Entry.VPath : path::parent_path(Entry.VPath); + if (Dir == DirStack.back()) { + if (!first_entry_is_directory) { + OS << ",\n"; + first_entry_is_directory = false; + } + } else { while (!DirStack.empty() && !containedIn(DirStack.back(), Dir)) { OS << "\n"; endDirectory(); diff --git a/llvm/unittests/Support/VirtualFileSystemTest.cpp b/llvm/unittests/Support/VirtualFileSystemTest.cpp index d61652bc0c50f..bd62811ae734a 100644 --- a/llvm/unittests/Support/VirtualFileSystemTest.cpp +++ b/llvm/unittests/Support/VirtualFileSystemTest.cpp @@ -2189,3 +2189,53 @@ TEST_F(VFSFromYAMLTest, WorkingDirectoryFallthroughInvalid) { Status = FS->status("foo/a"); ASSERT_TRUE(Status.getError()); } + +TEST_F(VFSFromYAMLTest, YAMLVFSWriterTest) { + ScopedDir TestDirectory("virtual-file-system-test", /*Unique*/ true); + ScopedDir _a(TestDirectory + "/a"); + ScopedFile _ab(TestDirectory + "/a/b", ""); + ScopedDir _c(TestDirectory + "/c"); + ScopedFile _cd(TestDirectory + "/c/d", ""); + ScopedDir _e(TestDirectory + "/e"); + ScopedDir _ef(TestDirectory + "/e/f"); + ScopedDir _g(TestDirectory + "/g"); + ScopedFile _h(TestDirectory + "/h", ""); + + vfs::YAMLVFSWriter VFSWriter; + VFSWriter.addDirectoryMapping(_a.Path, "//root/a"); + VFSWriter.addFileMapping(_ab.Path, "//root/a/b"); + VFSWriter.addFileMapping(_cd.Path, "//root/c/d"); + VFSWriter.addDirectoryMapping(_e.Path, "//root/e"); + VFSWriter.addDirectoryMapping(_ef.Path, "//root/e/f"); + VFSWriter.addFileMapping(_g.Path, "//root/g"); + VFSWriter.addDirectoryMapping(_h.Path, "//root/h"); + + std::string Buffer; + raw_string_ostream OS(Buffer); + VFSWriter.write(OS); + OS.flush(); + + IntrusiveRefCntPtr Lower(new ErrorDummyFileSystem()); + Lower->addDirectory("//root/"); + Lower->addDirectory("//root/a"); + Lower->addRegularFile("//root/a/b"); + Lower->addDirectory("//root/b"); + Lower->addDirectory("//root/c"); + Lower->addRegularFile("//root/c/d"); + Lower->addDirectory("//root/e"); + Lower->addDirectory("//root/e/f"); + Lower->addDirectory("//root/g"); + Lower->addRegularFile("//root/h"); + + IntrusiveRefCntPtr FS = getFromYAMLRawString(Buffer, Lower); + ASSERT_TRUE(FS.get() != nullptr); + + EXPECT_TRUE(FS->exists(_a.Path)); + EXPECT_TRUE(FS->exists(_ab.Path)); + EXPECT_TRUE(FS->exists(_c.Path)); + EXPECT_TRUE(FS->exists(_cd.Path)); + EXPECT_TRUE(FS->exists(_e.Path)); + EXPECT_TRUE(FS->exists(_ef.Path)); + EXPECT_TRUE(FS->exists(_g.Path)); + EXPECT_TRUE(FS->exists(_h.Path)); +} From 83dca65091967b4590cff8ac79227514e3c9c0b4 Mon Sep 17 00:00:00 2001 From: Jonas Devlieghere Date: Mon, 30 Mar 2020 12:58:21 -0700 Subject: [PATCH 08/13] Re-land "[FileCollector] Add a method to add a whole directory and it contents." Extend the FileCollector's API with addDirectory which adds a directory and its contents to the VFS mapping. Differential revision: https://reviews.llvm.org/D76671 (cherry picked from commit 4151f2d04ad8d829e16b83be734908fc38f977e9) --- llvm/include/llvm/Support/FileCollector.h | 18 +++++-- llvm/lib/Support/FileCollector.cpp | 56 ++++++++++++-------- llvm/unittests/Support/FileCollectorTest.cpp | 35 ++++++++++++ 3 files changed, 83 insertions(+), 26 deletions(-) diff --git a/llvm/include/llvm/Support/FileCollector.h b/llvm/include/llvm/Support/FileCollector.h index 079fe3efab9d3..1b6383ef8cc2d 100644 --- a/llvm/include/llvm/Support/FileCollector.h +++ b/llvm/include/llvm/Support/FileCollector.h @@ -18,7 +18,7 @@ #include namespace llvm { - +class FileCollectorFileSystem; /// Collects files into a directory and generates a mapping that can be used by /// the VFS. class FileCollector { @@ -26,9 +26,10 @@ class FileCollector { FileCollector(std::string Root, std::string OverlayRoot); void addFile(const Twine &file); + void addDirectory(const Twine &Dir); /// Write the yaml mapping (for the VFS) to the given file. - std::error_code writeMapping(StringRef mapping_file); + std::error_code writeMapping(StringRef MappingFile); /// Copy the files into the root directory. /// @@ -44,7 +45,7 @@ class FileCollector { std::shared_ptr Collector); private: - void addFileImpl(StringRef SrcPath); + friend FileCollectorFileSystem; bool markAsSeen(StringRef Path) { if (Path.empty()) @@ -55,10 +56,19 @@ class FileCollector { bool getRealPath(StringRef SrcPath, SmallVectorImpl &Result); void addFileToMapping(StringRef VirtualPath, StringRef RealPath) { - VFSWriter.addFileMapping(VirtualPath, RealPath); + if (sys::fs::is_directory(VirtualPath)) + VFSWriter.addDirectoryMapping(VirtualPath, RealPath); + else + VFSWriter.addFileMapping(VirtualPath, RealPath); } protected: + void addFileImpl(StringRef SrcPath); + + llvm::vfs::directory_iterator + addDirectoryImpl(const llvm::Twine &Dir, + IntrusiveRefCntPtr FS, std::error_code &EC); + /// Synchronizes adding files. std::mutex Mutex; diff --git a/llvm/lib/Support/FileCollector.cpp b/llvm/lib/Support/FileCollector.cpp index 47fca64137223..9d2485c151d67 100644 --- a/llvm/lib/Support/FileCollector.cpp +++ b/llvm/lib/Support/FileCollector.cpp @@ -61,13 +61,19 @@ bool FileCollector::getRealPath(StringRef SrcPath, return true; } -void FileCollector::addFile(const Twine &file) { +void FileCollector::addFile(const Twine &File) { std::lock_guard lock(Mutex); - std::string FileStr = file.str(); + std::string FileStr = File.str(); if (markAsSeen(FileStr)) addFileImpl(FileStr); } +void FileCollector::addDirectory(const Twine &Dir) { + assert(sys::fs::is_directory(Dir)); + std::error_code EC; + addDirectoryImpl(Dir, vfs::getRealFileSystem(), EC); +} + void FileCollector::addFileImpl(StringRef SrcPath) { // We need an absolute src path to append to the root. SmallString<256> AbsoluteSrc = SrcPath; @@ -101,6 +107,27 @@ void FileCollector::addFileImpl(StringRef SrcPath) { addFileToMapping(VirtualPath, DstPath); } +llvm::vfs::directory_iterator +FileCollector::addDirectoryImpl(const llvm::Twine &Dir, + IntrusiveRefCntPtr FS, + std::error_code &EC) { + auto It = FS->dir_begin(Dir, EC); + if (EC) + return It; + addFile(Dir); + for (; !EC && It != llvm::vfs::directory_iterator(); It.increment(EC)) { + if (It->type() == sys::fs::file_type::regular_file || + It->type() == sys::fs::file_type::directory_file || + It->type() == sys::fs::file_type::symlink_file) { + addFile(It->path()); + } + } + if (EC) + return It; + // Return a new iterator. + return FS->dir_begin(Dir, EC); +} + /// Set the access and modification time for the given file from the given /// status object. static std::error_code @@ -171,7 +198,7 @@ std::error_code FileCollector::copyFiles(bool StopOnError) { return {}; } -std::error_code FileCollector::writeMapping(StringRef mapping_file) { +std::error_code FileCollector::writeMapping(StringRef MappingFile) { std::lock_guard lock(Mutex); VFSWriter.setOverlayDir(OverlayRoot); @@ -179,7 +206,7 @@ std::error_code FileCollector::writeMapping(StringRef mapping_file) { VFSWriter.setUseExternalNames(false); std::error_code EC; - raw_fd_ostream os(mapping_file, EC, sys::fs::OF_Text); + raw_fd_ostream os(MappingFile, EC, sys::fs::OF_Text); if (EC) return EC; @@ -188,7 +215,7 @@ std::error_code FileCollector::writeMapping(StringRef mapping_file) { return {}; } -namespace { +namespace llvm { class FileCollectorFileSystem : public vfs::FileSystem { public: @@ -213,22 +240,7 @@ class FileCollectorFileSystem : public vfs::FileSystem { llvm::vfs::directory_iterator dir_begin(const llvm::Twine &Dir, std::error_code &EC) override { - auto It = FS->dir_begin(Dir, EC); - if (EC) - return It; - // Collect everything that's listed in case the user needs it. - Collector->addFile(Dir); - for (; !EC && It != llvm::vfs::directory_iterator(); It.increment(EC)) { - if (It->type() == sys::fs::file_type::regular_file || - It->type() == sys::fs::file_type::directory_file || - It->type() == sys::fs::file_type::symlink_file) { - Collector->addFile(It->path()); - } - } - if (EC) - return It; - // Return a new iterator. - return FS->dir_begin(Dir, EC); + return Collector->addDirectoryImpl(Dir, FS, EC); } std::error_code getRealPath(const Twine &Path, @@ -259,7 +271,7 @@ class FileCollectorFileSystem : public vfs::FileSystem { std::shared_ptr Collector; }; -} // end anonymous namespace +} // namespace llvm IntrusiveRefCntPtr FileCollector::createCollectorVFS(IntrusiveRefCntPtr BaseFS, diff --git a/llvm/unittests/Support/FileCollectorTest.cpp b/llvm/unittests/Support/FileCollectorTest.cpp index c6aeedd3865d3..c2c8bd53be388 100644 --- a/llvm/unittests/Support/FileCollectorTest.cpp +++ b/llvm/unittests/Support/FileCollectorTest.cpp @@ -120,6 +120,41 @@ TEST(FileCollectorTest, addFile) { EXPECT_FALSE(FileCollector.hasSeen("/path/to/d")); } +TEST(FileCollectorTest, addDirectory) { + ScopedDir file_root("file_root", true); + + llvm::SmallString<128> aaa = file_root.Path; + llvm::sys::path::append(aaa, "aaa"); + ScopedFile a(aaa.str()); + + llvm::SmallString<128> bbb = file_root.Path; + llvm::sys::path::append(bbb, "bbb"); + ScopedFile b(bbb.str()); + + llvm::SmallString<128> ccc = file_root.Path; + llvm::sys::path::append(ccc, "ccc"); + ScopedFile c(ccc.str()); + + std::string root_fs = std::string(file_root.Path.str()); + TestingFileCollector FileCollector(root_fs, root_fs); + + FileCollector.addDirectory(file_root.Path); + + // Make sure the root is correct. + EXPECT_EQ(FileCollector.Root, root_fs); + + // Make sure we've seen all the added files. + EXPECT_TRUE(FileCollector.hasSeen(a.Path)); + EXPECT_TRUE(FileCollector.hasSeen(b.Path)); + EXPECT_TRUE(FileCollector.hasSeen(c.Path)); + + // Make sure we've only seen the added files. + llvm::SmallString<128> ddd = file_root.Path; + llvm::sys::path::append(ddd, "ddd"); + ScopedFile d(ddd.str()); + EXPECT_FALSE(FileCollector.hasSeen(d.Path)); +} + TEST(FileCollectorTest, copyFiles) { ScopedDir file_root("file_root", true); ScopedFile a(file_root + "/aaa"); From 1d9a5bea33c6b9b94bb1a930df1655e3dd4188e7 Mon Sep 17 00:00:00 2001 From: Jonas Devlieghere Date: Mon, 30 Mar 2020 15:09:03 -0700 Subject: [PATCH 09/13] [lldb/Reproducers] Always collect the whole dSYM in the reproducer The FileCollector in LLDB collects every files that's used during a debug session when capture is enabled. This ensures that the reproducer only contains the files necessary to reproduce. This approach is not a good fit for the dSYM bundle, which is a directory on disk, but should be treated as a single unit. On macOS LLDB have automatically find the matching dSYM for a binary by its UUID. Having a incomplete dSYM in a reproducer can break debugging even when reproducers are disabled. This patch adds a was to specify a directory of interest to the reproducers. It is called from SymbolVendorMacOSX with the path of the dSYMs used by LLDB. Differential revision: https://reviews.llvm.org/D76672 (cherry picked from commit 38ddb49e5242920e44a982cff7bbe2e86bd23a69) --- lldb/include/lldb/Utility/Reproducer.h | 2 + .../MacOSX/SymbolVendorMacOSX.cpp | 247 +++++++++--------- lldb/source/Utility/Reproducer.cpp | 5 + lldb/test/Shell/Reproducer/TestDSYM.test | 11 + 4 files changed, 145 insertions(+), 120 deletions(-) create mode 100644 lldb/test/Shell/Reproducer/TestDSYM.test diff --git a/lldb/include/lldb/Utility/Reproducer.h b/lldb/include/lldb/Utility/Reproducer.h index 873ec3c76b529..cdf089d547cfd 100644 --- a/lldb/include/lldb/Utility/Reproducer.h +++ b/lldb/include/lldb/Utility/Reproducer.h @@ -98,6 +98,8 @@ class FileProvider : public Provider { return m_collector; } + void recordInterestingDirectory(const llvm::Twine &dir); + void Keep() override { auto mapping = GetRoot().CopyByAppendingPathComponent(Info::file); // Temporary files that are removed during execution can cause copy errors. diff --git a/lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp b/lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp index bb37d777906c7..090c139685f08 100644 --- a/lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp +++ b/lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp @@ -20,6 +20,7 @@ #include "lldb/Symbol/LocateSymbolFile.h" #include "lldb/Symbol/ObjectFile.h" #include "lldb/Target/Target.h" +#include "lldb/Utility/Reproducer.h" #include "lldb/Utility/StreamString.h" #include "lldb/Utility/Timer.h" @@ -143,6 +144,11 @@ SymbolVendorMacOSX::CreateInstance(const lldb::ModuleSP &module_sp, } if (dsym_fspec) { + // Compute dSYM root. + std::string dsym_root = dsym_fspec.GetPath(); + const size_t pos = dsym_root.find("/Contents/Resources/"); + dsym_root = pos != std::string::npos ? dsym_root.substr(0, pos) : ""; + DataBufferSP dsym_file_data_sp; lldb::offset_t dsym_file_data_offset = 0; dsym_objfile_sp = @@ -152,136 +158,132 @@ SymbolVendorMacOSX::CreateInstance(const lldb::ModuleSP &module_sp, if (UUIDsMatch(module_sp.get(), dsym_objfile_sp.get(), feedback_strm)) { // We need a XML parser if we hope to parse a plist... if (XMLDocument::XMLEnabled()) { - char dsym_path[PATH_MAX]; - if (module_sp->GetSourceMappingList().IsEmpty() && - dsym_fspec.GetPath(dsym_path, sizeof(dsym_path))) { + if (module_sp->GetSourceMappingList().IsEmpty()) { lldb_private::UUID dsym_uuid = dsym_objfile_sp->GetUUID(); if (dsym_uuid) { std::string uuid_str = dsym_uuid.GetAsString(); - if (!uuid_str.empty()) { - char *resources = strstr(dsym_path, "/Contents/Resources/"); - if (resources) { - char dsym_uuid_plist_path[PATH_MAX]; - resources[strlen("/Contents/Resources/")] = '\0'; - snprintf(dsym_uuid_plist_path, sizeof(dsym_uuid_plist_path), - "%s%s.plist", dsym_path, uuid_str.c_str()); - FileSpec dsym_uuid_plist_spec(dsym_uuid_plist_path); - if (FileSystem::Instance().Exists(dsym_uuid_plist_spec)) { - ApplePropertyList plist(dsym_uuid_plist_path); - if (plist) { - std::string DBGBuildSourcePath; - std::string DBGSourcePath; - - // DBGSourcePathRemapping is a dictionary in the plist - // with keys which are DBGBuildSourcePath file paths and - // values which are DBGSourcePath file paths - - StructuredData::ObjectSP plist_sp = - plist.GetStructuredData(); - if (plist_sp.get() && plist_sp->GetAsDictionary() && - plist_sp->GetAsDictionary()->HasKey( - "DBGSourcePathRemapping") && - plist_sp->GetAsDictionary() - ->GetValueForKey("DBGSourcePathRemapping") - ->GetAsDictionary()) { - - // If DBGVersion 1 or DBGVersion missing, ignore DBGSourcePathRemapping. - // If DBGVersion 2, strip last two components of path remappings from - // entries to fix an issue with a specific set of - // DBGSourcePathRemapping entries that lldb worked - // with. - // If DBGVersion 3, trust & use the source path remappings as-is. - // - - bool new_style_source_remapping_dictionary = false; - bool do_truncate_remapping_names = false; - std::string original_DBGSourcePath_value = - DBGSourcePath; - if (plist_sp->GetAsDictionary()->HasKey("DBGVersion")) { - std::string version_string = - plist_sp->GetAsDictionary() - ->GetValueForKey("DBGVersion") - ->GetStringValue(""); - if (!version_string.empty() && - isdigit(version_string[0])) { - int version_number = atoi(version_string.c_str()); - if (version_number > 1) { - new_style_source_remapping_dictionary = true; - } - if (version_number == 2) { - do_truncate_remapping_names = true; - } + if (!uuid_str.empty() && !dsym_root.empty()) { + char dsym_uuid_plist_path[PATH_MAX]; + snprintf(dsym_uuid_plist_path, sizeof(dsym_uuid_plist_path), + "%s/Contents/Resources/%s.plist", dsym_root.c_str(), + uuid_str.c_str()); + FileSpec dsym_uuid_plist_spec(dsym_uuid_plist_path); + if (FileSystem::Instance().Exists(dsym_uuid_plist_spec)) { + ApplePropertyList plist(dsym_uuid_plist_path); + if (plist) { + std::string DBGBuildSourcePath; + std::string DBGSourcePath; + + // DBGSourcePathRemapping is a dictionary in the plist + // with keys which are DBGBuildSourcePath file paths and + // values which are DBGSourcePath file paths + + StructuredData::ObjectSP plist_sp = + plist.GetStructuredData(); + if (plist_sp.get() && plist_sp->GetAsDictionary() && + plist_sp->GetAsDictionary()->HasKey( + "DBGSourcePathRemapping") && + plist_sp->GetAsDictionary() + ->GetValueForKey("DBGSourcePathRemapping") + ->GetAsDictionary()) { + + // If DBGVersion 1 or DBGVersion missing, ignore + // DBGSourcePathRemapping. If DBGVersion 2, strip last two + // components of path remappings from + // entries to fix an issue with a + // specific set of DBGSourcePathRemapping + // entries that lldb worked with. + // If DBGVersion 3, trust & use the source path remappings + // as-is. + // + + bool new_style_source_remapping_dictionary = false; + bool do_truncate_remapping_names = false; + std::string original_DBGSourcePath_value = DBGSourcePath; + if (plist_sp->GetAsDictionary()->HasKey("DBGVersion")) { + std::string version_string = + std::string(plist_sp->GetAsDictionary() + ->GetValueForKey("DBGVersion") + ->GetStringValue("")); + if (!version_string.empty() && + isdigit(version_string[0])) { + int version_number = atoi(version_string.c_str()); + if (version_number > 1) { + new_style_source_remapping_dictionary = true; + } + if (version_number == 2) { + do_truncate_remapping_names = true; } } + } - StructuredData::Dictionary *remappings_dict = - plist_sp->GetAsDictionary() - ->GetValueForKey("DBGSourcePathRemapping") - ->GetAsDictionary(); - remappings_dict->ForEach( - [&module_sp, new_style_source_remapping_dictionary, - original_DBGSourcePath_value, do_truncate_remapping_names]( - ConstString key, - StructuredData::Object *object) -> bool { - if (object && object->GetAsString()) { - - // key is DBGBuildSourcePath - // object is DBGSourcePath - std::string DBGSourcePath = - object->GetStringValue(); - if (!new_style_source_remapping_dictionary && - !original_DBGSourcePath_value.empty()) { - DBGSourcePath = original_DBGSourcePath_value; - } - if (DBGSourcePath[0] == '~') { - FileSpec resolved_source_path( - DBGSourcePath.c_str()); - FileSystem::Instance().Resolve( - resolved_source_path); - DBGSourcePath = - resolved_source_path.GetPath(); - } + StructuredData::Dictionary *remappings_dict = + plist_sp->GetAsDictionary() + ->GetValueForKey("DBGSourcePathRemapping") + ->GetAsDictionary(); + remappings_dict->ForEach( + [&module_sp, new_style_source_remapping_dictionary, + original_DBGSourcePath_value, + do_truncate_remapping_names]( + ConstString key, + StructuredData::Object *object) -> bool { + if (object && object->GetAsString()) { + + // key is DBGBuildSourcePath + // object is DBGSourcePath + std::string DBGSourcePath = + std::string(object->GetStringValue()); + if (!new_style_source_remapping_dictionary && + !original_DBGSourcePath_value.empty()) { + DBGSourcePath = original_DBGSourcePath_value; + } + if (DBGSourcePath[0] == '~') { + FileSpec resolved_source_path( + DBGSourcePath.c_str()); + FileSystem::Instance().Resolve( + resolved_source_path); + DBGSourcePath = resolved_source_path.GetPath(); + } + module_sp->GetSourceMappingList().Append( + key, ConstString(DBGSourcePath), true); + // With version 2 of DBGSourcePathRemapping, we + // can chop off the last two filename parts + // from the source remapping and get a more + // general source remapping that still works. + // Add this as another option in addition to + // the full source path remap. + if (do_truncate_remapping_names) { + FileSpec build_path(key.AsCString()); + FileSpec source_path(DBGSourcePath.c_str()); + build_path.RemoveLastPathComponent(); + build_path.RemoveLastPathComponent(); + source_path.RemoveLastPathComponent(); + source_path.RemoveLastPathComponent(); module_sp->GetSourceMappingList().Append( - key, ConstString(DBGSourcePath), true); - // With version 2 of DBGSourcePathRemapping, we - // can chop off the last two filename parts - // from the source remapping and get a more - // general source remapping that still works. - // Add this as another option in addition to - // the full source path remap. - if (do_truncate_remapping_names) { - FileSpec build_path(key.AsCString()); - FileSpec source_path(DBGSourcePath.c_str()); - build_path.RemoveLastPathComponent(); - build_path.RemoveLastPathComponent(); - source_path.RemoveLastPathComponent(); - source_path.RemoveLastPathComponent(); - module_sp->GetSourceMappingList().Append( - ConstString(build_path.GetPath().c_str()), - ConstString(source_path.GetPath().c_str()), true); - } + ConstString(build_path.GetPath().c_str()), + ConstString(source_path.GetPath().c_str()), + true); } - return true; - }); - } + } + return true; + }); + } - // If we have a DBGBuildSourcePath + DBGSourcePath pair, - // append those to the source path remappings. - - plist.GetValueAsString("DBGBuildSourcePath", - DBGBuildSourcePath); - plist.GetValueAsString("DBGSourcePath", DBGSourcePath); - if (!DBGBuildSourcePath.empty() && - !DBGSourcePath.empty()) { - if (DBGSourcePath[0] == '~') { - FileSpec resolved_source_path(DBGSourcePath.c_str()); - FileSystem::Instance().Resolve(resolved_source_path); - DBGSourcePath = resolved_source_path.GetPath(); - } - module_sp->GetSourceMappingList().Append( - ConstString(DBGBuildSourcePath), - ConstString(DBGSourcePath), true); + // If we have a DBGBuildSourcePath + DBGSourcePath pair, + // append those to the source path remappings. + + plist.GetValueAsString("DBGBuildSourcePath", + DBGBuildSourcePath); + plist.GetValueAsString("DBGSourcePath", DBGSourcePath); + if (!DBGBuildSourcePath.empty() && !DBGSourcePath.empty()) { + if (DBGSourcePath[0] == '~') { + FileSpec resolved_source_path(DBGSourcePath.c_str()); + FileSystem::Instance().Resolve(resolved_source_path); + DBGSourcePath = resolved_source_path.GetPath(); } + module_sp->GetSourceMappingList().Append( + ConstString(DBGBuildSourcePath), + ConstString(DBGSourcePath), true); } } } @@ -291,6 +293,11 @@ SymbolVendorMacOSX::CreateInstance(const lldb::ModuleSP &module_sp, } symbol_vendor->AddSymbolFileRepresentation(dsym_objfile_sp); + if (repro::Generator *g = + repro::Reproducer::Instance().GetGenerator()) { + repro::FileProvider &fp = g->GetOrCreate(); + fp.recordInterestingDirectory(dsym_root); + } return symbol_vendor; } } diff --git a/lldb/source/Utility/Reproducer.cpp b/lldb/source/Utility/Reproducer.cpp index 8987cfdac5565..6e3b8d269caf3 100644 --- a/lldb/source/Utility/Reproducer.cpp +++ b/lldb/source/Utility/Reproducer.cpp @@ -321,6 +321,11 @@ void WorkingDirectoryProvider::Keep() { os << m_cwd << "\n"; } +void FileProvider::recordInterestingDirectory(const llvm::Twine &dir) { + if (m_collector) + m_collector->addDirectory(dir); +} + void ProviderBase::anchor() {} char CommandProvider::ID = 0; char FileProvider::ID = 0; diff --git a/lldb/test/Shell/Reproducer/TestDSYM.test b/lldb/test/Shell/Reproducer/TestDSYM.test new file mode 100644 index 0000000000000..bddacda3242f9 --- /dev/null +++ b/lldb/test/Shell/Reproducer/TestDSYM.test @@ -0,0 +1,11 @@ +# REQUIRES: system-darwin +# Ensure that the reproducers captures the whole dSYM bundle. + +# RUN: rm -rf %t.repro +# RUN: %clang_host %S/Inputs/simple.c -g -o %t.out +# RUN: touch %t.out.dSYM/foo.bar + +# RUN: %lldb -x -b --capture --capture-path %t.repro %t.out -o 'b main' -o 'run' -o 'reproducer generate' + +# RUN: %lldb -b -o 'reproducer dump -p files -f %t.repro' | FileCheck %s --check-prefix FILES +# FILES: foo.bar From 724a15c66fecbbe15a2962c2c390db0150db8790 Mon Sep 17 00:00:00 2001 From: Adrian Prantl Date: Thu, 16 Apr 2020 17:06:48 -0700 Subject: [PATCH 10/13] Implement TypeSystemSwiftTypeRef::GetArrayElementType() (NFC) --- lldb/source/Symbol/SwiftASTContext.cpp | 4 +- lldb/source/Symbol/TypeSystemSwiftTypeRef.cpp | 45 ++++++++++++++----- .../Symbol/TestTypeSystemSwiftTypeRef.cpp | 3 ++ 3 files changed, 41 insertions(+), 11 deletions(-) diff --git a/lldb/source/Symbol/SwiftASTContext.cpp b/lldb/source/Symbol/SwiftASTContext.cpp index 70b94e9459114..d270f44e3930e 100644 --- a/lldb/source/Symbol/SwiftASTContext.cpp +++ b/lldb/source/Symbol/SwiftASTContext.cpp @@ -5175,7 +5175,9 @@ bool SwiftASTContext::IsArrayType(void *type, CompilerType *element_type_ptr, swift_can_type->getAs(); if (struct_type) { swift::StructDecl *struct_decl = struct_type->getDecl(); - if (strcmp(struct_decl->getName().get(), "Array") != 0) + llvm::StringRef name = struct_decl->getName().get(); + // This is sketchy, but it matches the behavior of GetArrayElementType(). + if (name != "Array" && name != "NativeArray" && name != "ArraySlice") return false; if (!struct_decl->getModuleContext()->isStdlibModule()) return false; diff --git a/lldb/source/Symbol/TypeSystemSwiftTypeRef.cpp b/lldb/source/Symbol/TypeSystemSwiftTypeRef.cpp index 1949ade4138ed..6786c8c7a72be 100644 --- a/lldb/source/Symbol/TypeSystemSwiftTypeRef.cpp +++ b/lldb/source/Symbol/TypeSystemSwiftTypeRef.cpp @@ -67,7 +67,7 @@ GetCanonicalNode(lldb_private::Module *M, swift::Demangle::Demangler &Dem, swift::STDLIB_NAME); e->addChild(module, Dem); NodePointer optional = - Dem.createNodeWithAllocatedText(Node::Kind::Module, "Optional"); + Dem.createNodeWithAllocatedText(Node::Kind::Identifier, "Optional"); e->addChild(optional, Dem); type->addChild(e, Dem); canonical->addChild(type, Dem); @@ -93,7 +93,7 @@ GetCanonicalNode(lldb_private::Module *M, swift::Demangle::Demangler &Dem, swift::STDLIB_NAME); structure->addChild(module, Dem); NodePointer array = - Dem.createNodeWithAllocatedText(Node::Kind::Module, "Array"); + Dem.createNodeWithAllocatedText(Node::Kind::Identifier, "Array"); structure->addChild(array, Dem); type->addChild(structure, Dem); canonical->addChild(type, Dem); @@ -121,7 +121,7 @@ GetCanonicalNode(lldb_private::Module *M, swift::Demangle::Demangler &Dem, swift::STDLIB_NAME); structure->addChild(module, Dem); NodePointer dict = - Dem.createNodeWithAllocatedText(Node::Kind::Module, "Dictionary"); + Dem.createNodeWithAllocatedText(Node::Kind::Identifier, "Dictionary"); structure->addChild(dict, Dem); type->addChild(structure, Dem); canonical->addChild(type, Dem); @@ -307,7 +307,24 @@ bool TypeSystemSwiftTypeRef::Verify(lldb::opaque_compiler_type_t type) { return true; const char *str = reinterpret_cast(type); - return SwiftLanguageRuntime::IsSwiftMangledName(str); + if (!SwiftLanguageRuntime::IsSwiftMangledName(str)) + return false; + + // Finally, check that the mangled name is canonical. + using namespace swift::Demangle; + Demangler dem; + NodePointer node = dem.demangleSymbol(str); + std::string remangled = mangleNode(node); + return remangled == std::string(str); +} + +namespace { +template bool Equivalent(T l, T r) { return l == r; } +/// Compare two swift types from different type systems by comparing their +/// (canonicalized) mangled name. +template <> bool Equivalent(CompilerType l, CompilerType r) { + return l.GetMangledTypeName() == r.GetMangledTypeName(); +} } #endif @@ -316,7 +333,7 @@ bool TypeSystemSwiftTypeRef::Verify(lldb::opaque_compiler_type_t type) { do { \ auto result = IMPL(); \ if (m_swift_ast_context) \ - assert(result == (EXPECTED) && \ + assert(Equivalent(result, (EXPECTED)) && \ "TypeSystemSwiftTypeRef diverges from SwiftASTContext"); \ return result; \ } while (0) @@ -341,6 +358,8 @@ swift::Demangle::NodePointer TypeSystemSwiftTypeRef::DemangleCanonicalType(swift::Demangle::Demangler &Dem, void *opaque_type) { using namespace swift::Demangle; + if (!opaque_type) + return nullptr; NodePointer node = GetCanonicalDemangleTree(GetModule(), Dem, AsMangledName(opaque_type)); @@ -379,14 +398,15 @@ bool TypeSystemSwiftTypeRef::IsArrayType(void *type, CompilerType *element_type, node->getChild(0)->getText() != swift::STDLIB_NAME || node->getChild(1)->getKind() != Node::Kind::Identifier || !node->getChild(1)->hasText() || - node->getChild(1)->getText() != "Array") + (node->getChild(1)->getText() != "Array" && + node->getChild(1)->getText() != "NativeArray" && + node->getChild(1)->getText() != "ArraySlice")) return false; if (elem_node->getNumChildren() != 1 || elem_node->getKind() != Node::Kind::TypeList) return false; elem_node = elem_node->getFirstChild(); - if (element_type) *element_type = RemangleAsType(Dem, elem_node); @@ -573,14 +593,19 @@ lldb::TypeClass TypeSystemSwiftTypeRef::GetTypeClass(void *type) { // Creating related types CompilerType TypeSystemSwiftTypeRef::GetArrayElementType(void *type, uint64_t *stride) { - return m_swift_ast_context->GetArrayElementType(ReconstructType(type), - stride); + auto impl = [&]() { + CompilerType element_type; + IsArrayType(type, &element_type, nullptr, nullptr); + return element_type; + }; + VALIDATE_AND_RETURN(impl, m_swift_ast_context->GetArrayElementType( + ReconstructType(type), nullptr)); } CompilerType TypeSystemSwiftTypeRef::GetCanonicalType(void *type) { return m_swift_ast_context->GetCanonicalType(ReconstructType(type)); } int TypeSystemSwiftTypeRef::GetFunctionArgumentCount(void *type) { - auto impl = [&]() { return GetNumberOfFunctionArguments(type); }; + auto impl = [&]() -> int { return GetNumberOfFunctionArguments(type); }; VALIDATE_AND_RETURN(impl, m_swift_ast_context->GetFunctionArgumentCount( ReconstructType(type))); } diff --git a/lldb/unittests/Symbol/TestTypeSystemSwiftTypeRef.cpp b/lldb/unittests/Symbol/TestTypeSystemSwiftTypeRef.cpp index 6b81b9f110c97..55261d494d953 100644 --- a/lldb/unittests/Symbol/TestTypeSystemSwiftTypeRef.cpp +++ b/lldb/unittests/Symbol/TestTypeSystemSwiftTypeRef.cpp @@ -92,6 +92,9 @@ TEST_F(TestTypeSystemSwiftTypeRef, Array) { b.Node(Node::Kind::TypeList, b.IntType()))); CompilerType int_array = GetCompilerType(b.Mangle(n)); ASSERT_TRUE(int_array.IsArrayType(nullptr, nullptr, nullptr)); + NodePointer int_node = b.GlobalTypeMangling(b.IntType()); + CompilerType int_type = GetCompilerType(b.Mangle(int_node)); + ASSERT_EQ(int_array.GetArrayElementType(nullptr), int_type); } TEST_F(TestTypeSystemSwiftTypeRef, Function) { From c9e31f78b125d69677ac4d02d80ed7eccb07dc7f Mon Sep 17 00:00:00 2001 From: Jonas Devlieghere Date: Tue, 21 Apr 2020 11:14:30 -0700 Subject: [PATCH 11/13] [apple/stable/20200108] Fix ambiguity Fix error: conditional expression is ambiguous; 'const std::string' (aka 'const basic_string, allocator >') can be converted to 'llvm::StringRef' and vice versa. --- llvm/lib/Support/VirtualFileSystem.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/llvm/lib/Support/VirtualFileSystem.cpp b/llvm/lib/Support/VirtualFileSystem.cpp index 2b899e90e0ae7..ba35d251d6bb2 100644 --- a/llvm/lib/Support/VirtualFileSystem.cpp +++ b/llvm/lib/Support/VirtualFileSystem.cpp @@ -2010,8 +2010,8 @@ void JSONWriter::write(ArrayRef Entries, if (!Entries.empty()) { const YAMLVFSEntry &Entry = Entries.front(); bool first_entry_is_directory = Entry.IsDirectory; - StringRef Dir = - first_entry_is_directory ? Entry.VPath : path::parent_path(Entry.VPath); + StringRef Dir = first_entry_is_directory ? StringRef(Entry.VPath) + : path::parent_path(Entry.VPath); startDirectory(Dir); StringRef RPath = Entry.RPath; @@ -2026,8 +2026,8 @@ void JSONWriter::write(ArrayRef Entries, writeEntry(path::filename(Entry.VPath), RPath); for (const auto &Entry : Entries.slice(1)) { - StringRef Dir = - Entry.IsDirectory ? Entry.VPath : path::parent_path(Entry.VPath); + StringRef Dir = Entry.IsDirectory ? StringRef(Entry.VPath) + : path::parent_path(Entry.VPath); if (Dir == DirStack.back()) { if (!first_entry_is_directory) { OS << ",\n"; From e8a928d0b5479f017a905f7bea9499a5c08b43b1 Mon Sep 17 00:00:00 2001 From: Dan Liew Date: Mon, 20 Apr 2020 17:22:35 -0700 Subject: [PATCH 12/13] Unbreak ASan runtime in the simulators. Summary: 861b69faee5df8d4e13ef316c7474a10e4069e81 (rdar://problem/58789439) while fixing symbolization for TSan completely broke ASan's runtime for the simulators. The problem with the previous patch is that the memory passed to `putenv()` was poisoned and when passed to `putenv()` it tripped an interceptor for `strchr()` which saw the memory was poisoned and raised an ASan issue. The memory was poisoned because `AtosSymbolizerProcess` objects are created using ASan's internal allocator. Memory from this allocator gets poisoned with `kAsanInternalHeapMagic`. To workaround this, this patch makes the memory for the environment variable entry a global variable that isn't poisoned. This pass also adds a `DCHECK(getenv(K_ATOS_ENV_VAR))` because the following DCHECK would crash because `internal_strcmp()` doesn't work on nullptr. rdar://problem/62067724 Reviewers: kubamracek, yln Subscribers: #sanitizers, llvm-commits Tags: #sanitizers Differential Revision: https://reviews.llvm.org/D78525 (cherry picked from commit 7039773b240b6eee00b5be5bc325c5c57501788a) --- .../sanitizer_symbolizer_mac.cpp | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp index 4623c3f76b120..cc233408d0ceb 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp +++ b/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cpp @@ -53,6 +53,11 @@ bool DlAddrSymbolizer::SymbolizeData(uptr addr, DataInfo *datainfo) { #define K_ATOS_ENV_VAR "__check_mach_ports_lookup" +// This cannot live in `AtosSymbolizerProcess` because instances of that object +// are allocated by the internal allocator which under ASan is poisoned with +// kAsanInternalHeapMagic. +static char kAtosMachPortEnvEntry[] = K_ATOS_ENV_VAR "=000000000000000"; + class AtosSymbolizerProcess : public SymbolizerProcess { public: explicit AtosSymbolizerProcess(const char *path) @@ -69,7 +74,7 @@ class AtosSymbolizerProcess : public SymbolizerProcess { // We use `putenv()` rather than `setenv()` so that we can later directly // write into the storage without LibC getting involved to change what the // variable is set to - int result = putenv(mach_port_env_var_entry_); + int result = putenv(kAtosMachPortEnvEntry); CHECK_EQ(result, 0); } } @@ -95,12 +100,13 @@ class AtosSymbolizerProcess : public SymbolizerProcess { // for our task port. We can't call `setenv()` here because it might call // malloc/realloc. To avoid that we instead update the // `mach_port_env_var_entry_` variable with our current PID. - uptr count = internal_snprintf(mach_port_env_var_entry_, - sizeof(mach_port_env_var_entry_), + uptr count = internal_snprintf(kAtosMachPortEnvEntry, + sizeof(kAtosMachPortEnvEntry), K_ATOS_ENV_VAR "=%s", pid_str_); CHECK_GE(count, sizeof(K_ATOS_ENV_VAR) + internal_strlen(pid_str_)); // Document our assumption but without calling `getenv()` in normal // builds. + DCHECK(getenv(K_ATOS_ENV_VAR)); DCHECK_EQ(internal_strcmp(getenv(K_ATOS_ENV_VAR), pid_str_), 0); } @@ -127,9 +133,10 @@ class AtosSymbolizerProcess : public SymbolizerProcess { } char pid_str_[16]; - // Space for `\0` in `kAtosEnvVar_` is reused for `=`. - char mach_port_env_var_entry_[sizeof(K_ATOS_ENV_VAR) + sizeof(pid_str_)] = - K_ATOS_ENV_VAR "=0"; + // Space for `\0` in `K_ATOS_ENV_VAR` is reused for `=`. + static_assert(sizeof(kAtosMachPortEnvEntry) == + (sizeof(K_ATOS_ENV_VAR) + sizeof(pid_str_)), + "sizes should match"); }; #undef K_ATOS_ENV_VAR From b5749104908b85d6058821fa0a6a9f7297e6a8d9 Mon Sep 17 00:00:00 2001 From: Dan Liew Date: Mon, 20 Apr 2020 18:27:43 -0700 Subject: [PATCH 13/13] Add missing call to `Symbolizer::LateInitialize()` in UBSan's standalone init. Summary: This fixes symbolization in Standalone UBSan mode for the Darwin simulators. 861b69faee5df8d4e13ef316c7474a10e4069e81 (rdar://problem/58789439) tried to fix symbolization for all sanitizers on Darwin simulators but unfortunately it only fixed the problem for TSan. For UBSan in standalone mode the fix wasn't sufficient because UBSan's standalone init doesn't call `Symbolizer::LateInitialize()` like ASan and TSan do. This meant that `AtosSymbolizerProcess::LateInitialize()` was never being called before `AtosSymbolizerProcess::StartSymbolizerSubprocess()` which breaks an invariant we expect to hold. The missing call to `Symbolizer::LateInitialize()` during UBSan's standalone init seems like an accidently omission so this patch simply adds it. rdar://problem/62083617 Reviewers: vitalybuka, kubamracek, yln, samsonov Subscribers: #sanitizers, llvm-commits Tags: #sanitizers Differential Revision: https://reviews.llvm.org/D78530 (cherry picked from commit 564530e50ad4870801a2080a08645cc1cc2df805) --- compiler-rt/lib/ubsan/ubsan_init.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/compiler-rt/lib/ubsan/ubsan_init.cpp b/compiler-rt/lib/ubsan/ubsan_init.cpp index 1a3b7d3726743..26b6227aa484b 100644 --- a/compiler-rt/lib/ubsan/ubsan_init.cpp +++ b/compiler-rt/lib/ubsan/ubsan_init.cpp @@ -41,6 +41,7 @@ static void CommonStandaloneInit() { AndroidLogInit(); InitializeCoverage(common_flags()->coverage, common_flags()->coverage_dir); CommonInit(); + Symbolizer::LateInitialize(); } void __ubsan::InitAsStandalone() {