-fdebug-prefix-map=: make the last win when multiple prefixes match

[MaskRay]

Summary

For
clang -c -g -fdebug-prefix-map=a/b=y -fdebug-prefix-map=a=x a/b/c.c,
we apply the longest prefix substitution, but
GCC has always been picking the last applicable option (a=x, see
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109591).

I feel that GCC's behavior is reasonable given the convention that the last
value wins for the same option.

Before D49466, Clang appeared to apply the shortest prefix substitution,
which likely made the least sense.

[joerg]

For me long matching prefix makes more sense, but if the same prefix is used multiple times, the last option should win.

[scott.linder]

clang/include/clang/Basic/CodeGenOptions.h

209

What benefit does forcing allocation have? Why not use the default, or switch to std::vector?

clang/lib/CodeGen/CGDebugInfo.cpp

72–77

Can you use the member-initializer-list here?

[MaskRay]

clang/include/clang/Basic/CodeGenOptions.h

209

This doesn't force allocation. I use inline element size 0 to make the member variable smaller than a std::vector.
std::vector likely compiles to larger code.

clang/lib/CodeGen/CGDebugInfo.cpp

72–77

No, this doesn't work. We convert a vector of std::string to a vector of StringRef.

[scott.linder]

clang/include/clang/Basic/CodeGenOptions.h

209

Sorry, I just didn't realize this was idiomatic (i.e. I hadn't read https://llvm.org/docs/ProgrammersManual.html#vector), and I didn't see other similar uses in the file.

There are 15 std::vector members of CodeGenOptions, but no SmallVector<_, 0> members; maybe the rest can be converted in an NFC patch, so things are consistent?

clang/lib/CodeGen/CGDebugInfo.cpp

72–77

If all we are doing is creating another vector which shares the underlying strings with the original, why not just save a reference to the original vector? Is the cost of the extra dereference when accessing it greater than the cost of traversing it plus the extra storage for the StringRefs?

It seems like the original utility was just to get the std::map sorting behavior, which we no longer need.

[MaskRay]

remove a DebugPrefixMap variable

[MaskRay]

clang/include/clang/Basic/CodeGenOptions.h

209

It seems that SmallVector is used more frequently but std::vector is used as well. I'd prefer that we don't change the types for existing variables...

% rg SmallVector lib -l | wc -l => 439
% rg std::vector lib -l | wc -l => 255

clang/lib/CodeGen/CGDebugInfo.cpp

72–77

Thanks for the giving this more attention. Actually I found that the variable can be removed. Removed:)

[scott.linder]

LGTM, thank you!

Does this warrant a release note, as it is changing the behavior in a backwards-incompatible manner? I do think changing to match GCC is worthwhile, even if it means a change in behavior for Clang.

I also don't think the "longest first" heuristic is useful enough to outweigh the benefits of behaving like GCC; it seems like the user can always sort their own options to get the same effect.

[MaskRay]

Update HelpText

[MaskRay]

In D148975#4296625, @scott.linder wrote:

LGTM, thank you!

Does this warrant a release note, as it is changing the behavior in a backwards-incompatible manner? I do think changing to match GCC is worthwhile, even if it means a change in behavior for Clang.

I also don't think the "longest first" heuristic is useful enough to outweigh the benefits of behaving like GCC; it seems like the user can always sort their own options to get the same effect.

Thank you! I feel that specifying multiple -fdebug-prefix-map= is a very uncommon situation, so a release note entry is likely overkill.
I updated the HelpText, though.

In D148975#4289159, @joerg wrote:

For me long matching prefix makes more sense, but if the same prefix is used multiple times, the last option should win.

"if the same prefix is used multiple times, the last option should win." I think this interpretation will add some complexity, e.g. whether we prefer -fdebug-prefix-map=a/=b or -fdebug-prefix-map=a=c. GCC's current rule (simple) seems quite reasonable.

From the discussion on https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109591 , GCC will likely remain the current behavior and will improve the documentation.

[dyung]

@MaskRay , a test you modified, debug-prefix-map.s is failing on the PS5 Windows bot. Can you take a look?

https://lab.llvm.org/buildbot/#/builders/216/builds/20460

[dyung]

llvm/test/MC/ELF/debug-prefix-map.s

54

This might need another regex for the path separator between src_root and bar.
https://lab.llvm.org/buildbot/#/builders/216/builds/20460/steps/7/logs/FAIL__LLVM__debug-prefix-map_s

<stdin>:11:2: note: possible intended match here
 DW_AT_comp_dir [DW_FORM_string] ("/src_root\\bar")
 ^

[dankm]

In D148975#4296895, @MaskRay wrote:

Thank you! I feel that specifying multiple -fdebug-prefix-map= is a very uncommon situation, so a release note entry is likely overkill.
I updated the HelpText, though.

The Yocto project and my WIP changes against FreeBSD both specify multiple -fdebug-prefix-map= options, but neither depend on the longest-match first semantics. They did at one point, but GCC behaved differently.

So, I suppose this LGTM.

clang/include/clang/Basic/CodeGenOptions.h

public:
   /// The string to embed in the debug information for the compile unit, if
   /// non-empty.
   std::string DwarfDebugFlags;
 
   /// The string containing the commandline for the llvm.commandline metadata,
   /// if non-empty.
   std::string RecordCommandLine;
 
-  std::map<std::string, std::string> DebugPrefixMap;
+  llvm::SmallVector<std::pair<std::string, std::string>, 0> DebugPrefixMap;
   std::map<std::string, std::string> CoveragePrefixMap;
 
   /// The ABI to use for passing floating point arguments.
   std::string FloatABI;
 
   /// The file to use for dumping bug report by `Debugify` for original
   /// debug info.
   std::string DIBugsReportFilePath;

clang/include/clang/Driver/Options.td

defm split_dwarf_inlining : BoolFOption<"split-dwarf-inlining",
   PosFlag<SetTrue, [CC1Option], "Provide minimal debug info in the object/executable"
           " to facilitate online symbolication/stack traces in the absence of"
           " .dwo/.dwp files when using Split DWARF">>;
 def fdebug_default_version: Joined<["-"], "fdebug-default-version=">, Group<f_Group>,
   HelpText<"Default DWARF version to use, if a -g option caused DWARF debug info to be produced">;
 def fdebug_prefix_map_EQ
   : Joined<["-"], "fdebug-prefix-map=">, Group<f_Group>,
     Flags<[CC1Option,CC1AsOption]>,
-    HelpText<"remap file source paths in debug info">;
+    MetaVarName<"<old>=<new>">,
+    HelpText<"For paths in debug info, remap directory <old> to <new>. If multiple options match a path, the last option wins">;
 def fcoverage_prefix_map_EQ
   : Joined<["-"], "fcoverage-prefix-map=">, Group<f_Group>,
     Flags<[CC1Option]>,
     HelpText<"remap file source paths in coverage mapping">;
 def ffile_prefix_map_EQ
   : Joined<["-"], "ffile-prefix-map=">, Group<f_Group>,
     HelpText<"remap file source paths in debug info, predefined preprocessor "
              "macros and __builtin_FILE(). Implies -ffile-reproducible.">;

clang/lib/CodeGen/CGDebugInfo.h

#define EXT_OPAQUE_TYPE(ExtType, Id, Ext) \
   llvm::DIType *Id##Ty = nullptr;
 #include "clang/Basic/OpenCLExtensionTypes.def"
 #define WASM_TYPE(Name, Id, SingletonId) llvm::DIType *SingletonId = nullptr;
 #include "clang/Basic/WebAssemblyReferenceTypes.def"
 
   /// Cache of previously constructed Types.
   llvm::DenseMap<const void *, llvm::TrackingMDRef> TypeCache;
 
-  std::map<llvm::StringRef, llvm::StringRef, std::greater<llvm::StringRef>>
-      DebugPrefixMap;
-
   /// Cache that maps VLA types to size expressions for that type,
   /// represented by instantiated Metadata nodes.
   llvm::SmallDenseMap<QualType, llvm::Metadata *> SizeExprCache;
 
   /// Callbacks to use when printing names and types.
   class PrintingCallbacks final : public clang::PrintingCallbacks {
     const CGDebugInfo &Self;
 

clang/lib/CodeGen/CGDebugInfo.cpp

 static uint32_t getTypeAlignIfRequired(QualType Ty, const ASTContext &Ctx) {
   return getTypeAlignIfRequired(Ty.getTypePtr(), Ctx);
 }
 
 static uint32_t getDeclAlignIfRequired(const Decl *D, const ASTContext &Ctx) {
   return D->hasAttr<AlignedAttr>() ? D->getMaxAlignment() : 0;
 }
 
 CGDebugInfo::CGDebugInfo(CodeGenModule &CGM)
     : CGM(CGM), DebugKind(CGM.getCodeGenOpts().getDebugInfo()),
       DebugTypeExtRefs(CGM.getCodeGenOpts().DebugTypeExtRefs),
       DBuilder(CGM.getModule()) {
-  for (const auto &KV : CGM.getCodeGenOpts().DebugPrefixMap)
-    DebugPrefixMap[KV.first] = KV.second;
   CreateCompileUnit();
 }
 
 CGDebugInfo::~CGDebugInfo() {
   assert(LexicalBlockStack.empty() &&
          "Region stack mismatch, stack not empty!");
 }
 
 ApplyDebugLocation::ApplyDebugLocation(CodeGenFunction &CGF,
                                        SourceLocation TemporaryLocation)
if (llvm::sys::path::is_absolute(RemappedFile)) {
     File = RemappedFile;
   }
   llvm::DIFile *F = DBuilder.createFile(File, Dir, CSInfo, Source);
   DIFileCache[FileName.data()].reset(F);
   return F;
 }
 
 std::string CGDebugInfo::remapDIPath(StringRef Path) const {
-  if (DebugPrefixMap.empty())
-    return Path.str();
-
   SmallString<256> P = Path;
-  for (const auto &Entry : DebugPrefixMap)
-    if (llvm::sys::path::replace_path_prefix(P, Entry.first, Entry.second))
+  for (auto &[From, To] : llvm::reverse(CGM.getCodeGenOpts().DebugPrefixMap))
+    if (llvm::sys::path::replace_path_prefix(P, From, To))
       break;
   return P.str().str();
 }
 
 unsigned CGDebugInfo::getLineNumber(SourceLocation Loc) {
   if (Loc.isInvalid())
     return 0;
   SourceManager &SM = CGM.getContext().getSourceManager();

clang/lib/Frontend/CompilerInvocation.cpp

if (A->getOption().matches(OPT_fuse_ctor_homing) &&
       Opts.setDebugInfo(llvm::codegenoptions::DebugInfoConstructor);
     if (A->getOption().matches(OPT_fno_use_ctor_homing) &&
         Opts.getDebugInfo() == llvm::codegenoptions::DebugInfoConstructor)
       Opts.setDebugInfo(llvm::codegenoptions::LimitedDebugInfo);
   }
 
   for (const auto &Arg : Args.getAllArgValues(OPT_fdebug_prefix_map_EQ)) {
     auto Split = StringRef(Arg).split('=');
-    Opts.DebugPrefixMap.insert(
-        {std::string(Split.first), std::string(Split.second)});
+    Opts.DebugPrefixMap.emplace_back(Split.first, Split.second);
   }
 
   for (const auto &Arg : Args.getAllArgValues(OPT_fcoverage_prefix_map_EQ)) {
     auto Split = StringRef(Arg).split('=');
     Opts.CoveragePrefixMap.insert(
         {std::string(Split.first), std::string(Split.second)});
   }
 

clang/test/CodeGen/debug-prefix-map.c

 // RUN: %clang_cc1 -debug-info-kind=standalone -fdebug-prefix-map=%p=%{fs-src-root}UNLIKELY_PATH%{fs-sep}empty %s -emit-llvm -o - | FileCheck %s -check-prefix CHECK-NO-MAIN-FILE-NAME
 // RUN: %clang_cc1 -debug-info-kind=standalone -fdebug-prefix-map=%p=%{fs-src-root}UNLIKELY_PATH=empty %s -emit-llvm -o - | FileCheck %s -check-prefix CHECK-EVIL
 // RUN: %clang_cc1 -debug-info-kind=standalone -fdebug-prefix-map=%p=%{fs-src-root}UNLIKELY_PATH%{fs-sep}empty %s -emit-llvm -o - -main-file-name debug-prefix-map.c | FileCheck %s
 // RUN: %clang_cc1 -debug-info-kind=standalone -fdebug-prefix-map=%p=%{fs-src-root}UNLIKELY_PATH%{fs-sep}empty %s -emit-llvm -o - -fdebug-compilation-dir %p | FileCheck %s -check-prefix CHECK-COMPILATION-DIR
 // RUN: %clang_cc1 -debug-info-kind=standalone -fdebug-prefix-map=%p=%{fs-src-root}UNLIKELY_PATH%{fs-sep}empty %s -emit-llvm -o - -isysroot %p -debugger-tuning=lldb | FileCheck %s -check-prefix CHECK-SYSROOT
 // RUN: %clang -g -fdebug-prefix-map=%p=%{fs-src-root}UNLIKELY_PATH%{fs-sep}empty -S -c %s -emit-llvm -o - | FileCheck %s
 // RUN: %clang -g -ffile-prefix-map=%p=%{fs-src-root}UNLIKELY_PATH%{fs-sep}empty -S -c %s -emit-llvm -o - | FileCheck %s
 
 // RUN: %clang -g -fdebug-prefix-map=%p=./UNLIKELY_PATH/empty -S -c %s -emit-llvm -o - | FileCheck %s --check-prefix=CHECK-REL
 // RUN: %clang -g -ffile-prefix-map=%p=./UNLIKELY_PATH/empty -S -c %s -emit-llvm -o - | FileCheck %s --check-prefix=CHECK-REL
 
+// RUN: rm -rf %t && mkdir -p %t/a/b && cp %s %t/a/b/c.c
+// RUN: %clang_cc1 -emit-llvm -debug-info-kind=standalone -I%S -fdebug-prefix-map=%t/a/b=y -fdebug-prefix-map=%t/a=x %t/a/b/c.c -o - | FileCheck %s --check-prefix=CHECK-X
+// RUN: %clang_cc1 -emit-llvm -debug-info-kind=standalone -I%S -fdebug-prefix-map=%t/a=x -fdebug-prefix-map=%t/a/b=y %t/a/b/c.c -o - | FileCheck %s --check-prefix=CHECK-Y
+
 #include "Inputs/stdio.h"
 
 int main(int argc, char **argv) {
   (void)argc;
   (void)argv;
   return 0;
 }
 

 // CHECK-COMPILATION-DIR: !DIFile(filename: "{{.*}}", directory: "{{/|.:\\\\}}UNLIKELY_PATH{{/|\\\\}}empty")
 // CHECK-COMPILATION-DIR: !DIFile(filename: "{{.*}}Inputs{{/|\\\\}}stdio.h", directory: "{{/|.:\\\\}}UNLIKELY_PATH{{/|\\\\}}empty")
 // CHECK-COMPILATION-DIR-NOT: !DIFile(filename:
 // CHECK-SYSROOT: !DICompileUnit({{.*}}sysroot: "{{/|.:\\\\}}UNLIKELY_PATH{{/|\\\\}}empty"
 
 // CHECK-REL: !DIFile(filename: "./UNLIKELY_PATH/empty{{/|\\\\}}{{.*}}",
 // CHECK-REL: !DIFile(filename: "./UNLIKELY_PATH/empty{{/|\\\\}}{{.*}}Inputs/stdio.h",
 // CHECK-REL-SAME:    directory: ""
+
+// CHECK-X: !DIFile(filename: "x{{/|\\\\}}b{{/|\\\\}}c.c", directory: "")
+// CHECK-Y: !DIFile(filename: "y{{/|\\\\}}c.c", directory: "")

clang/tools/driver/cc1as_main.cpp

struct AssemblerInvocation {
   unsigned SaveTemporaryLabels : 1;
   unsigned GenDwarfForAssembly : 1;
   unsigned RelaxELFRelocations : 1;
   unsigned Dwarf64 : 1;
   unsigned DwarfVersion;
   std::string DwarfDebugFlags;
   std::string DwarfDebugProducer;
   std::string DebugCompilationDir;
-  std::map<const std::string, const std::string> DebugPrefixMap;
+  llvm::SmallVector<std::pair<std::string, std::string>, 0> DebugPrefixMap;
   llvm::DebugCompressionType CompressDebugSections =
       llvm::DebugCompressionType::None;
   std::string MainFileName;
   std::string SplitDwarfOutput;
 
   /// @}
   /// @name Frontend Options
   /// @{
Opts.DwarfDebugProducer =
       std::string(Args.getLastArgValue(OPT_dwarf_debug_producer));
   if (const Arg *A = Args.getLastArg(options::OPT_ffile_compilation_dir_EQ,
                                      options::OPT_fdebug_compilation_dir_EQ))
     Opts.DebugCompilationDir = A->getValue();
   Opts.MainFileName = std::string(Args.getLastArgValue(OPT_main_file_name));
 
   for (const auto &Arg : Args.getAllArgValues(OPT_fdebug_prefix_map_EQ)) {
     auto Split = StringRef(Arg).split('=');
-    Opts.DebugPrefixMap.insert(
-        {std::string(Split.first), std::string(Split.second)});
+    Opts.DebugPrefixMap.emplace_back(Split.first, Split.second);
   }
 
   // Frontend Options
   if (Args.hasArg(OPT_INPUT)) {
     bool First = true;
     for (const Arg *A : Args.filtered(OPT_INPUT)) {
       if (First) {
         Opts.InputFile = A->getValue();

llvm/include/llvm/MC/MCContext.h

private:
   /// catch errors if .secure_log_unique appears twice without
   /// .secure_log_reset appearing between them.
   bool SecureLogUsed = false;
 
   /// The compilation directory to use for DW_AT_comp_dir.
   SmallString<128> CompilationDir;
 
   /// Prefix replacement map for source file information.
-  std::map<std::string, const std::string, std::greater<>> DebugPrefixMap;
+  SmallVector<std::pair<std::string, std::string>, 0> DebugPrefixMap;
 
   /// The main file name if passed in explicitly.
   std::string MainFileName;
 
   /// The dwarf file and directory tables from the dwarf .file directive.
   /// We now emit a line table for each compile unit. To reduce the prologue
   /// size of each line table, the files and directories used by each compile
   /// unit are separated.

llvm/lib/MC/MCContext.cpp

 }
 
 MCSubtargetInfo &MCContext::getSubtargetCopy(const MCSubtargetInfo &STI) {
   return *new (MCSubtargetAllocator.Allocate()) MCSubtargetInfo(STI);
 }
 
 void MCContext::addDebugPrefixMapEntry(const std::string &From,
                                        const std::string &To) {
-  DebugPrefixMap.insert(std::make_pair(From, To));
+  DebugPrefixMap.emplace_back(From, To);
 }
 
 void MCContext::remapDebugPath(SmallVectorImpl<char> &Path) {
-  for (const auto &[From, To] : DebugPrefixMap)
+  for (const auto &[From, To] : llvm::reverse(DebugPrefixMap))
     if (llvm::sys::path::replace_path_prefix(Path, From, To))
       break;
 }
 
 void MCContext::RemapDebugPaths() {
   const auto &DebugPrefixMap = this->DebugPrefixMap;
   if (DebugPrefixMap.empty())
     return;

llvm/test/MC/ELF/debug-prefix-map.s

 # RUN: rm -rf %t.foo && mkdir -p %t.foo/bar && cd %t.foo
 # RUN: cp %s %t.foo/src.s && cp %s %t.foo/bar/src.s
 
 # RUN: llvm-mc -triple=x86_64 -g -dwarf-version=4 src.s -filetype=obj -o nomap.4.o
 # RUN: llvm-dwarfdump -v -debug-info -debug-line nomap.4.o | FileCheck --check-prefix=NO_MAP_V4 %s
 # RUN: llvm-mc -triple=x86_64 -g -dwarf-version=5 src.s -filetype=obj -o nomap.5.o
 # RUN: llvm-dwarfdump -v -debug-info -debug-line nomap.5.o | FileCheck --check-prefix=NO_MAP_V5 %s
 
 # RUN: llvm-mc -triple=x86_64 -g -dwarf-version=4 src.s -filetype=obj -o map.4.o -fdebug-prefix-map=%t.foo=src_root
 # RUN: llvm-dwarfdump -v -debug-info -debug-line map.4.o | FileCheck --check-prefix=MAP_V4 %s
 # RUN: llvm-mc -triple=x86_64 -g -dwarf-version=5 src.s -filetype=obj -o map.5.o -fdebug-prefix-map=%t.foo=src_root
 # RUN: llvm-dwarfdump -v -debug-info -debug-line map.5.o | FileCheck --check-prefix=MAP_V5 %s
 
 # RUN: cd %t.foo/bar
 # RUN: llvm-mc -triple=x86_64 -g -dwarf-version=4 %t.foo%{fs-sep}bar%{fs-sep}src.s -filetype=obj -o mapabs.4.o -fdebug-prefix-map=%t.foo=/broken_root -fdebug-prefix-map=%t.foo%{fs-sep}bar=/src_root
 # RUN: llvm-dwarfdump -v -debug-info -debug-line mapabs.4.o | FileCheck --check-prefix=MAPABS_V4 %s
-# RUN: llvm-mc -triple=x86_64 -g -dwarf-version=5 %t.foo%{fs-sep}bar%{fs-sep}src.s -filetype=obj -o mapabs.5.o -fdebug-prefix-map=%t.foo%{fs-sep}bar=/src_root -fdebug-prefix-map=%t.foo=/broken_root
+# RUN: llvm-mc -triple=x86_64 -g -dwarf-version=5 %t.foo%{fs-sep}bar%{fs-sep}src.s -filetype=obj -o mapabs.5.o -fdebug-prefix-map=%t.foo=/broken_root -fdebug-prefix-map=%t.foo%{fs-sep}bar=/src_root
 # RUN: llvm-dwarfdump -v -debug-info -debug-line mapabs.5.o | FileCheck --check-prefix=MAPABS_V5 %s
 
+## The last -fdebug-prefix-map= wins even if the prefix is shorter.
+# RUN: llvm-mc -triple=x86_64 -g -dwarf-version=5 %t.foo%{fs-sep}bar%{fs-sep}src.s -filetype=obj -o mapabs.5.o -fdebug-prefix-map=%t.foo%{fs-sep}bar=/broken_root -fdebug-prefix-map=%t.foo=/src_root
+# RUN: llvm-dwarfdump -v -debug-info -debug-line mapabs.5.o | FileCheck --check-prefix=MAPABS_V5_A %s
+
 f:
   nop
 
 # NO_MAP_V4:      DW_AT_comp_dir [DW_FORM_string] ("{{.*}}.foo")
 # NO_MAP_V4:      file_names[  1]:
 # NO_MAP_V4-NEXT:   name: "src.s"
 
 # NO_MAP_V5:      DW_AT_comp_dir [DW_FORM_string] ("{{.*}}.foo")

 # MAPABS_V4:      DW_AT_comp_dir [DW_FORM_string] ("{{(/|\\)+}}src_root")
 # MAPABS_V4:      DW_AT_decl_file [DW_FORM_data4] ("/src_root{{(/|\\)+}}src.s")
 # MAPABS_V4-NOT:  .foo
 
 # MAPABS_V5:      DW_AT_name [DW_FORM_string] ("src.s")
 # MAPABS_V5:      DW_AT_comp_dir [DW_FORM_string] ("{{(/|\\)+}}src_root")
 # MAPABS_V5:      DW_AT_decl_file [DW_FORM_data4] ("/src_root{{(/|\\)+}}src.s")
 # MAPABS_V5:      include_directories[ 0] = .debug_line_str[0x00000000] = "/src_root"
+
+# MAPABS_V5_A:    DW_AT_comp_dir [DW_FORM_string] ("{{(/|\\)+}}src_root/bar")
+# MAPABS_V5_A:    DW_AT_decl_file [DW_FORM_data4] ("/src_root{{(/|\\)+}}bar{{(/|\\)+}}src.s")