From c86ef59d2aac654468204f6a0da21c396f826eb5 Mon Sep 17 00:00:00 2001 From: Jonas Devlieghere Date: Fri, 22 Sep 2023 11:27:33 -0700 Subject: [PATCH] [dsymutil] Remove paper trail warnings (#67041) Remove the paper trail warning from dsymutil and the DWARF linker. The original purpose of this functionality was to leave a paper trail in the output artifact about missing object files. The current implementation however has diverged and is the source of a pretty serious bug: - In the debug map parser, missing object files are not the only warnings we emit. When paper trail warnings are enabled, all of them end up in the dSYM which wasn't the goal. - When warnings are associated with a object file in the debug map, it is skipped by the DWARF linker. This only makes sense if the object file is missing and is obviously incorrect for any other type of warning (such as a missing symbol). The combination of the two means that we can generate broken DWARF when the feature is enabled. AFAIK it was only used by Apple and nobody I spoke to has relied on it, so rather than fixing the broken behavior I propose we remove it. (cherry picked from commit 697b34fd966bef0a94e67870a5517e8102751c46) --- llvm/docs/CommandGuide/dsymutil.rst | 7 -- llvm/include/llvm/DWARFLinker/DWARFLinker.h | 15 +---- llvm/include/llvm/DWARFLinker/DWARFStreamer.h | 3 - .../llvm/DWARFLinkerParallel/DWARFFile.h | 7 +- llvm/lib/DWARFLinker/DWARFLinker.cpp | 66 ------------------- llvm/lib/DWARFLinker/DWARFStreamer.cpp | 12 ---- .../dsymutil/X86/papertrail-warnings.test | 30 --------- llvm/test/tools/dsymutil/cmdline.test | 1 - llvm/tools/dsymutil/DwarfLinkerForBinary.cpp | 6 +- llvm/tools/dsymutil/MachODebugMapParser.cpp | 20 ++---- llvm/tools/dsymutil/Options.td | 4 -- llvm/tools/dsymutil/dsymutil.cpp | 14 +--- llvm/tools/dsymutil/dsymutil.h | 3 +- llvm/tools/llvm-dwarfutil/DebugInfoLinker.cpp | 6 +- 14 files changed, 14 insertions(+), 180 deletions(-) delete mode 100644 llvm/test/tools/dsymutil/X86/papertrail-warnings.test diff --git a/llvm/docs/CommandGuide/dsymutil.rst b/llvm/docs/CommandGuide/dsymutil.rst index 0cc2a472bbd06..02243e227a24d 100644 --- a/llvm/docs/CommandGuide/dsymutil.rst +++ b/llvm/docs/CommandGuide/dsymutil.rst @@ -100,13 +100,6 @@ OPTIONS Specifies an alternate ``path`` to place the dSYM bundle. The default dSYM bundle path is created by appending ``.dSYM`` to the executable name. -.. option:: --papertrail - - When running dsymutil as part of your build system, it can be desirable for - warnings to be part of the end product, rather than just being emitted to the - output stream. When enabled warnings are embedded in the linked DWARF debug - information. - .. option:: --remarks-drop-without-debug Drop remarks without valid debug locations. Without this flags, all remarks are kept. diff --git a/llvm/include/llvm/DWARFLinker/DWARFLinker.h b/llvm/include/llvm/DWARFLinker/DWARFLinker.h index 5dce990adf02d..9543546935eef 100644 --- a/llvm/include/llvm/DWARFLinker/DWARFLinker.h +++ b/llvm/include/llvm/DWARFLinker/DWARFLinker.h @@ -99,9 +99,6 @@ class DwarfEmitter { public: virtual ~DwarfEmitter(); - /// Emit DIE containing warnings. - virtual void emitPaperTrailWarningsDie(DIE &Die) = 0; - /// Emit section named SecName with data SecData. virtual void emitSectionContents(StringRef SecData, StringRef SecName) = 0; @@ -267,10 +264,9 @@ using UnitListTy = std::vector>; class DWARFFile { public: DWARFFile(StringRef Name, std::unique_ptr Dwarf, - std::unique_ptr Addresses, - const std::vector &Warnings) + std::unique_ptr Addresses) : FileName(Name), Dwarf(std::move(Dwarf)), - Addresses(std::move(Addresses)), Warnings(Warnings) {} + Addresses(std::move(Addresses)) {} /// The object file name. StringRef FileName; @@ -280,9 +276,6 @@ class DWARFFile { /// Helpful address information(list of valid address ranges, relocations). std::unique_ptr Addresses; - - /// Warnings for this object file. - const std::vector &Warnings; }; typedef std::map swiftInterfacesMap; @@ -502,10 +495,6 @@ class DWARFLinker { ErrorHandler(Warning, File.FileName, DIE); } - /// Emit warnings as Dwarf compile units to leave a trail after linking. - bool emitPaperTrailWarnings(const DWARFFile &File, - OffsetsStringPool &StringPool); - void copyInvariantDebugSection(DWARFContext &Dwarf); /// Keep information for referenced clang module: already loaded DWARF info diff --git a/llvm/include/llvm/DWARFLinker/DWARFStreamer.h b/llvm/include/llvm/DWARFLinker/DWARFStreamer.h index ec2281d462b6b..91933fa6fc947 100644 --- a/llvm/include/llvm/DWARFLinker/DWARFStreamer.h +++ b/llvm/include/llvm/DWARFLinker/DWARFStreamer.h @@ -72,9 +72,6 @@ class DwarfStreamer : public DwarfEmitter { void emitAbbrevs(const std::vector> &Abbrevs, unsigned DwarfVersion) override; - /// Emit DIE containing warnings. - void emitPaperTrailWarningsDie(DIE &Die) override; - /// Emit contents of section SecName From Obj. void emitSectionContents(StringRef SecData, StringRef SecName) override; diff --git a/llvm/include/llvm/DWARFLinkerParallel/DWARFFile.h b/llvm/include/llvm/DWARFLinkerParallel/DWARFFile.h index c20d59f9771d9..9e2d7bbf2361d 100644 --- a/llvm/include/llvm/DWARFLinkerParallel/DWARFFile.h +++ b/llvm/include/llvm/DWARFLinkerParallel/DWARFFile.h @@ -29,11 +29,9 @@ class DWARFFile { DWARFFile(StringRef Name, std::unique_ptr Dwarf, std::unique_ptr Addresses, - const std::vector &Warnings, UnloadCallbackTy UnloadFunc = nullptr) : FileName(Name), Dwarf(std::move(Dwarf)), - Addresses(std::move(Addresses)), Warnings(Warnings), - UnloadFunc(UnloadFunc) { + Addresses(std::move(Addresses)), UnloadFunc(UnloadFunc) { if (this->Dwarf) Endianess = this->Dwarf->isLittleEndian() ? support::endianness::little : support::endianness::big; @@ -48,9 +46,6 @@ class DWARFFile { /// Helpful address information(list of valid address ranges, relocations). std::unique_ptr Addresses; - /// Warnings for object file. - const std::vector &Warnings; - /// Endiannes of source DWARF information. support::endianness Endianess = support::endianness::little; diff --git a/llvm/lib/DWARFLinker/DWARFLinker.cpp b/llvm/lib/DWARFLinker/DWARFLinker.cpp index ca81e36ad3ca8..c15d916aaf13a 100644 --- a/llvm/lib/DWARFLinker/DWARFLinker.cpp +++ b/llvm/lib/DWARFLinker/DWARFLinker.cpp @@ -2561,69 +2561,6 @@ uint64_t DWARFLinker::DIECloner::cloneAllCompileUnits( return OutputDebugInfoSize - StartOutputDebugInfoSize; } -bool DWARFLinker::emitPaperTrailWarnings(const DWARFFile &File, - OffsetsStringPool &StringPool) { - - if (File.Warnings.empty()) - return false; - - DIE *CUDie = DIE::get(DIEAlloc, dwarf::DW_TAG_compile_unit); - CUDie->setOffset(11); - StringRef Producer; - StringRef WarningHeader; - - switch (DwarfLinkerClientID) { - case DwarfLinkerClient::Dsymutil: - Producer = StringPool.internString("dsymutil"); - WarningHeader = "dsymutil_warning"; - break; - - default: - Producer = StringPool.internString("dwarfopt"); - WarningHeader = "dwarfopt_warning"; - break; - } - - StringRef FileName = StringPool.internString(File.FileName); - CUDie->addValue(DIEAlloc, dwarf::DW_AT_producer, dwarf::DW_FORM_strp, - DIEInteger(StringPool.getStringOffset(Producer))); - DIEBlock *String = new (DIEAlloc) DIEBlock(); - DIEBlocks.push_back(String); - for (auto &C : FileName) - String->addValue(DIEAlloc, dwarf::Attribute(0), dwarf::DW_FORM_data1, - DIEInteger(C)); - String->addValue(DIEAlloc, dwarf::Attribute(0), dwarf::DW_FORM_data1, - DIEInteger(0)); - - CUDie->addValue(DIEAlloc, dwarf::DW_AT_name, dwarf::DW_FORM_string, String); - for (const auto &Warning : File.Warnings) { - DIE &ConstDie = CUDie->addChild(DIE::get(DIEAlloc, dwarf::DW_TAG_constant)); - ConstDie.addValue(DIEAlloc, dwarf::DW_AT_name, dwarf::DW_FORM_strp, - DIEInteger(StringPool.getStringOffset(WarningHeader))); - ConstDie.addValue(DIEAlloc, dwarf::DW_AT_artificial, dwarf::DW_FORM_flag, - DIEInteger(1)); - ConstDie.addValue(DIEAlloc, dwarf::DW_AT_const_value, dwarf::DW_FORM_strp, - DIEInteger(StringPool.getStringOffset(Warning))); - } - unsigned Size = 4 /* FORM_strp */ + FileName.size() + 1 + - File.Warnings.size() * (4 + 1 + 4) + 1 /* End of children */; - DIEAbbrev Abbrev = CUDie->generateAbbrev(); - assignAbbrev(Abbrev); - CUDie->setAbbrevNumber(Abbrev.getNumber()); - Size += getULEB128Size(Abbrev.getNumber()); - // Abbreviation ordering needed for classic compatibility. - for (auto &Child : CUDie->children()) { - Abbrev = Child.generateAbbrev(); - assignAbbrev(Abbrev); - Child.setAbbrevNumber(Abbrev.getNumber()); - Size += getULEB128Size(Abbrev.getNumber()); - } - CUDie->setSize(Size); - TheDwarfEmitter->emitPaperTrailWarningsDie(*CUDie); - - return true; -} - void DWARFLinker::copyInvariantDebugSection(DWARFContext &Dwarf) { TheDwarfEmitter->emitSectionContents(Dwarf.getDWARFObj().getLocSection().Data, "debug_loc"); @@ -2687,9 +2624,6 @@ Error DWARFLinker::link() { outs() << "OBJECT FILE: " << OptContext.File.FileName << "\n"; } - if (emitPaperTrailWarnings(OptContext.File, DebugStrPool)) - continue; - if (!OptContext.File.Dwarf) continue; diff --git a/llvm/lib/DWARFLinker/DWARFStreamer.cpp b/llvm/lib/DWARFLinker/DWARFStreamer.cpp index fbd89dcf1ca13..c60d5aa4528af 100644 --- a/llvm/lib/DWARFLinker/DWARFStreamer.cpp +++ b/llvm/lib/DWARFLinker/DWARFStreamer.cpp @@ -234,18 +234,6 @@ void DwarfStreamer::emitSectionContents(StringRef SecData, StringRef SecName) { } } -/// Emit DIE containing warnings. -void DwarfStreamer::emitPaperTrailWarningsDie(DIE &Die) { - switchToDebugInfoSection(/* Version */ 2); - auto &Asm = getAsmPrinter(); - Asm.emitInt32(11 + Die.getSize() - 4); - Asm.emitInt16(2); - Asm.emitInt32(0); - Asm.emitInt8(MC->getTargetTriple().isArch64Bit() ? 8 : 4); - DebugInfoSectionSize += 11; - emitDIE(Die); -} - /// Emit the debug_str section stored in \p Pool. void DwarfStreamer::emitStrings(const NonRelocatableStringpool &Pool) { Asm->OutStreamer->switchSection(MOFI->getDwarfStrSection()); diff --git a/llvm/test/tools/dsymutil/X86/papertrail-warnings.test b/llvm/test/tools/dsymutil/X86/papertrail-warnings.test deleted file mode 100644 index 1678adb8ae7b7..0000000000000 --- a/llvm/test/tools/dsymutil/X86/papertrail-warnings.test +++ /dev/null @@ -1,30 +0,0 @@ -RUN: env RC_DEBUG_OPTIONS=1 dsymutil -f %p/../Inputs/basic.macho.x86_64 -o - | llvm-dwarfdump -v - | FileCheck -DMSG=%errc_ENOENT %s - -CHECK: .debug_info contents: -CHECK: Compile Unit: -CHECK: DW_TAG_compile_unit [1] * -CHECK: DW_AT_producer {{.*}}"dsymutil -CHECK: DW_AT_name {{.*}}"/Inputs/basic1.macho.x86_64.o" -CHECK: DW_TAG_constant [2] -CHECK: DW_AT_name {{.*}}"dsymutil_warning" -CHECK: DW_AT_artificial [DW_FORM_flag] (0x01) -CHECK: DW_AT_const_value {{.*}}"unable to open object file: [[MSG]]" -CHECK: NULL -CHECK: Compile Unit: -CHECK: DW_TAG_compile_unit [1] * -CHECK: DW_AT_producer {{.*}}"dsymutil -CHECK: DW_AT_name {{.*}}"/Inputs/basic2.macho.x86_64.o" -CHECK: DW_TAG_constant [2] -CHECK: DW_AT_name {{.*}}"dsymutil_warning" -CHECK: DW_AT_artificial [DW_FORM_flag] (0x01) -CHECK: DW_AT_const_value {{.*}}"unable to open object file: [[MSG]]" -CHECK: NULL -CHECK: Compile Unit: -CHECK: DW_TAG_compile_unit [1] * -CHECK: DW_AT_producer {{.*}}"dsymutil -CHECK: DW_AT_name {{.*}}"/Inputs/basic3.macho.x86_64.o" -CHECK: DW_TAG_constant [2] -CHECK: DW_AT_name {{.*}}"dsymutil_warning" -CHECK: DW_AT_artificial [DW_FORM_flag] (0x01) -CHECK: DW_AT_const_value {{.*}}"unable to open object file: [[MSG]]" -CHECK: NULL diff --git a/llvm/test/tools/dsymutil/cmdline.test b/llvm/test/tools/dsymutil/cmdline.test index dc716cf25b21f..2317852f3c489 100644 --- a/llvm/test/tools/dsymutil/cmdline.test +++ b/llvm/test/tools/dsymutil/cmdline.test @@ -21,7 +21,6 @@ CHECK: -object-prefix-map CHECK: -oso-prepend-path CHECK: -out CHECK: {{-o }} -CHECK: -papertrail CHECK: -remarks-drop-without-debug CHECK: -remarks-output-format CHECK: -remarks-prepend-path diff --git a/llvm/tools/dsymutil/DwarfLinkerForBinary.cpp b/llvm/tools/dsymutil/DwarfLinkerForBinary.cpp index 4b281d5f19801..0e383b4f53b14 100644 --- a/llvm/tools/dsymutil/DwarfLinkerForBinary.cpp +++ b/llvm/tools/dsymutil/DwarfLinkerForBinary.cpp @@ -234,8 +234,7 @@ DwarfLinkerForBinary::loadObject(const DebugMapObject &Obj, if (ErrorOrObj) { Res = std::make_unique( Obj.getObjectFilename(), DWARFContext::create(*ErrorOrObj), - std::make_unique(*this, *ErrorOrObj, Obj), - Obj.empty() ? Obj.getWarnings() : EmptyWarnings); + std::make_unique(*this, *ErrorOrObj, Obj)); Error E = RL.link(*ErrorOrObj); if (Error NewE = handleErrors( @@ -768,8 +767,7 @@ bool DwarfLinkerForBinary::linkImpl( OnCUDieLoaded); } else { ObjectsForLinking.push_back(std::make_unique( - Obj->getObjectFilename(), nullptr, nullptr, - Obj->empty() ? Obj->getWarnings() : EmptyWarnings)); + Obj->getObjectFilename(), nullptr, nullptr)); GeneralLinker->addObjectFile(*ObjectsForLinking.back()); } } diff --git a/llvm/tools/dsymutil/MachODebugMapParser.cpp b/llvm/tools/dsymutil/MachODebugMapParser.cpp index d3bda338a9b32..d9bf2301e21e5 100644 --- a/llvm/tools/dsymutil/MachODebugMapParser.cpp +++ b/llvm/tools/dsymutil/MachODebugMapParser.cpp @@ -28,11 +28,9 @@ class MachODebugMapParser { public: MachODebugMapParser(llvm::IntrusiveRefCntPtr VFS, StringRef BinaryPath, ArrayRef Archs, - StringRef PathPrefix = "", - bool PaperTrailWarnings = false, bool Verbose = false) + StringRef PathPrefix = "", bool Verbose = false) : BinaryPath(std::string(BinaryPath)), Archs(Archs.begin(), Archs.end()), - PathPrefix(std::string(PathPrefix)), - PaperTrailWarnings(PaperTrailWarnings), BinHolder(VFS, Verbose), + PathPrefix(std::string(PathPrefix)), BinHolder(VFS, Verbose), CurrentDebugMapObject(nullptr), SkipDebugMapObject(false) {} /// Parses and returns the DebugMaps of the input binary. The binary contains @@ -50,7 +48,6 @@ class MachODebugMapParser { std::string BinaryPath; SmallVector Archs; std::string PathPrefix; - bool PaperTrailWarnings; /// Owns the MemoryBuffer for the main binary. BinaryHolder BinHolder; @@ -137,13 +134,6 @@ class MachODebugMapParser { << MachOUtils::getArchName( Result->getTriple().getArchName()) << ") " << File << " " << Msg << "\n"; - - if (PaperTrailWarnings) { - if (!File.empty()) - Result->addDebugMapObject(File, sys::TimePoint()); - if (Result->end() != Result->begin()) - (*--Result->end())->addWarning(Msg.str()); - } } }; @@ -704,13 +694,11 @@ namespace dsymutil { llvm::ErrorOr>> parseDebugMap(llvm::IntrusiveRefCntPtr VFS, StringRef InputFile, ArrayRef Archs, - StringRef PrependPath, bool PaperTrailWarnings, bool Verbose, - bool InputIsYAML) { + StringRef PrependPath, bool Verbose, bool InputIsYAML) { if (InputIsYAML) return DebugMap::parseYAMLDebugMap(InputFile, PrependPath, Verbose); - MachODebugMapParser Parser(VFS, InputFile, Archs, PrependPath, - PaperTrailWarnings, Verbose); + MachODebugMapParser Parser(VFS, InputFile, Archs, PrependPath, Verbose); return Parser.parse(); } diff --git a/llvm/tools/dsymutil/Options.td b/llvm/tools/dsymutil/Options.td index 9b0b31b4b0e1d..79f04fdfb0360 100644 --- a/llvm/tools/dsymutil/Options.td +++ b/llvm/tools/dsymutil/Options.td @@ -69,10 +69,6 @@ def yaml_input: Flag<["-", "--"], "y">, HelpText<"Treat the input file is a YAML debug map rather than a binary.">, Group; -def papertrail: F<"papertrail">, - HelpText<"Embed warnings in the linked DWARF debug info.">, - Group; - def assembly: Flag<["-", "--"], "S">, HelpText<"Output textual assembly instead of a binary dSYM companion file.">, Group; diff --git a/llvm/tools/dsymutil/dsymutil.cpp b/llvm/tools/dsymutil/dsymutil.cpp index 6f1e06cf2f659..085c5c5bbecee 100644 --- a/llvm/tools/dsymutil/dsymutil.cpp +++ b/llvm/tools/dsymutil/dsymutil.cpp @@ -106,7 +106,6 @@ struct DsymutilOptions { bool DumpStab = false; bool Flat = false; bool InputIsYAMLDebugMap = false; - bool PaperTrailWarnings = false; bool ForceKeepFunctionForStatic = false; std::string SymbolMap; std::string OutputFile; @@ -197,11 +196,6 @@ static Error verifyOptions(const DsymutilOptions &Options) { "cannot use -o with multiple inputs in flat mode.", errc::invalid_argument); - if (Options.PaperTrailWarnings && Options.InputIsYAMLDebugMap) - return make_error( - "paper trail warnings are not supported for YAML input.", - errc::invalid_argument); - if (!Options.ReproducerPath.empty() && Options.ReproMode != ReproducerMode::Use) return make_error( @@ -303,7 +297,6 @@ static Expected getOptions(opt::InputArgList &Args) { Options.DumpStab = Args.hasArg(OPT_symtab); Options.Flat = Args.hasArg(OPT_flat); Options.InputIsYAMLDebugMap = Args.hasArg(OPT_yaml_input); - Options.PaperTrailWarnings = Args.hasArg(OPT_papertrail); if (Expected Verify = getVerifyKind(Args)) { Options.Verify = *Verify; @@ -389,9 +382,6 @@ static Expected getOptions(opt::InputArgList &Args) { if (Options.DumpDebugMap || Options.LinkOpts.Verbose) Options.LinkOpts.Threads = 1; - if (getenv("RC_DEBUG_OPTIONS")) - Options.PaperTrailWarnings = true; - if (opt::Arg *RemarksPrependPath = Args.getLastArg(OPT_remarks_prepend_path)) Options.LinkOpts.RemarksPrependPath = RemarksPrependPath->getValue(); @@ -671,8 +661,8 @@ int main(int argc, char **argv) { auto DebugMapPtrsOrErr = parseDebugMap(Options.LinkOpts.VFS, InputFile, Options.Archs, - Options.LinkOpts.PrependPath, Options.PaperTrailWarnings, - Options.LinkOpts.Verbose, Options.InputIsYAMLDebugMap); + Options.LinkOpts.PrependPath, Options.LinkOpts.Verbose, + Options.InputIsYAMLDebugMap); if (auto EC = DebugMapPtrsOrErr.getError()) { WithColor::error() << "cannot parse the debug map for '" << InputFile diff --git a/llvm/tools/dsymutil/dsymutil.h b/llvm/tools/dsymutil/dsymutil.h index e6799286610df..9fff51aed8fcf 100644 --- a/llvm/tools/dsymutil/dsymutil.h +++ b/llvm/tools/dsymutil/dsymutil.h @@ -37,8 +37,7 @@ class BinaryHolder; ErrorOr>> parseDebugMap(llvm::IntrusiveRefCntPtr VFS, StringRef InputFile, ArrayRef Archs, - StringRef PrependPath, bool PaperTrailWarnings, bool Verbose, - bool InputIsYAML); + StringRef PrependPath, bool Verbose, bool InputIsYAML); /// Dump the symbol table. bool dumpStab(llvm::IntrusiveRefCntPtr VFS, diff --git a/llvm/tools/llvm-dwarfutil/DebugInfoLinker.cpp b/llvm/tools/llvm-dwarfutil/DebugInfoLinker.cpp index 47a23e8448cc4..a9d0aeed53722 100644 --- a/llvm/tools/llvm-dwarfutil/DebugInfoLinker.cpp +++ b/llvm/tools/llvm-dwarfutil/DebugInfoLinker.cpp @@ -322,7 +322,6 @@ Error linkDebugInfoImpl(object::ObjectFile &File, const Options &Options, DebugInfoLinker->setUpdateIndexTablesOnly(!Options.DoGarbageCollection); std::vector> ObjectsForLinking(1); - std::vector EmptyWarnings; // Add object files to the DWARFLinker. std::unique_ptr Context = DWARFContext::create(File); @@ -330,9 +329,8 @@ Error linkDebugInfoImpl(object::ObjectFile &File, const Options &Options, std::make_unique>(*Context, Options, File)); - ObjectsForLinking[0] = - std::make_unique(File.getFileName(), std::move(Context), - std::move(AddressesMap), EmptyWarnings); + ObjectsForLinking[0] = std::make_unique( + File.getFileName(), std::move(Context), std::move(AddressesMap)); uint16_t MaxDWARFVersion = 0; std::function OnCUDieLoaded =