[PATCH] D148975: -fdebug-prefix-map=: make the last win when multiple prefixes match

2023-04-27 Thread Dan McGregor via Phabricator via cfe-commits 
dankm added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148975/new/

https://reviews.llvm.org/D148975

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148975: -fdebug-prefix-map=: make the last win when multiple prefixes match

2023-04-25 Thread Douglas Yung via Phabricator via cfe-commits 
dyung added inline comments.



Comment at: llvm/test/MC/ELF/debug-prefix-map.s:54
+
+# 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")

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
```
:11:2: note: possible intended match here
 DW_AT_comp_dir [DW_FORM_string] ("/src_root\\bar")
 ^
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148975/new/

https://reviews.llvm.org/D148975

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148975: -fdebug-prefix-map=: make the last win when multiple prefixes match

2023-04-25 Thread Douglas Yung via Phabricator via cfe-commits 
dyung added a comment.

@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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148975/new/

https://reviews.llvm.org/D148975

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148975: -fdebug-prefix-map=: make the last win when multiple prefixes match

2023-04-25 Thread Fangrui Song via Phabricator via cfe-commits 
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGdaad48d6b236: -fdebug-prefix-map=: make the last win when 
multiple prefixes match (authored by MaskRay).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148975/new/

https://reviews.llvm.org/D148975

Files:
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/debug-prefix-map.c
  clang/tools/driver/cc1as_main.cpp
  llvm/include/llvm/MC/MCContext.h
  llvm/lib/MC/MCContext.cpp
  llvm/test/MC/ELF/debug-prefix-map.s

Index: llvm/test/MC/ELF/debug-prefix-map.s
===
--- llvm/test/MC/ELF/debug-prefix-map.s
+++ llvm/test/MC/ELF/debug-prefix-map.s
@@ -14,9 +14,13 @@
 # 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
 
@@ -46,3 +50,6 @@
 # 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[0x] = "/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")
Index: llvm/lib/MC/MCContext.cpp
===
--- llvm/lib/MC/MCContext.cpp
+++ llvm/lib/MC/MCContext.cpp
@@ -881,11 +881,11 @@
 
 void MCContext::addDebugPrefixMapEntry(const std::string ,
const std::string ) {
-  DebugPrefixMap.insert(std::make_pair(From, To));
+  DebugPrefixMap.emplace_back(From, To);
 }
 
 void MCContext::remapDebugPath(SmallVectorImpl ) {
-  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;
 }
Index: llvm/include/llvm/MC/MCContext.h
===
--- llvm/include/llvm/MC/MCContext.h
+++ llvm/include/llvm/MC/MCContext.h
@@ -190,7 +190,7 @@
   SmallString<128> CompilationDir;
 
   /// Prefix replacement map for source file information.
-  std::map> DebugPrefixMap;
+  SmallVector, 0> DebugPrefixMap;
 
   /// The main file name if passed in explicitly.
   std::string MainFileName;
Index: clang/tools/driver/cc1as_main.cpp
===
--- clang/tools/driver/cc1as_main.cpp
+++ clang/tools/driver/cc1as_main.cpp
@@ -97,7 +97,7 @@
   std::string DwarfDebugFlags;
   std::string DwarfDebugProducer;
   std::string DebugCompilationDir;
-  std::map DebugPrefixMap;
+  llvm::SmallVector, 0> DebugPrefixMap;
   llvm::DebugCompressionType CompressDebugSections =
   llvm::DebugCompressionType::None;
   std::string MainFileName;
@@ -275,8 +275,7 @@
 
   for (const auto  : 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
Index: clang/test/CodeGen/debug-prefix-map.c
===
--- clang/test/CodeGen/debug-prefix-map.c
+++ clang/test/CodeGen/debug-prefix-map.c
@@ -9,6 +9,10 @@
 // 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 - |

[PATCH] D148975: -fdebug-prefix-map=: make the last win when multiple prefixes match

2023-04-25 Thread Fangrui Song via Phabricator via cfe-commits 
MaskRay added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148975/new/

https://reviews.llvm.org/D148975

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148975: -fdebug-prefix-map=: make the last win when multiple prefixes match

2023-04-25 Thread Fangrui Song via Phabricator via cfe-commits 
MaskRay updated this revision to Diff 516924.
MaskRay marked an inline comment as done.
MaskRay added a comment.

Update HelpText


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148975/new/

https://reviews.llvm.org/D148975

Files:
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/debug-prefix-map.c
  clang/tools/driver/cc1as_main.cpp
  llvm/include/llvm/MC/MCContext.h
  llvm/lib/MC/MCContext.cpp
  llvm/test/MC/ELF/debug-prefix-map.s

Index: llvm/test/MC/ELF/debug-prefix-map.s
===
--- llvm/test/MC/ELF/debug-prefix-map.s
+++ llvm/test/MC/ELF/debug-prefix-map.s
@@ -14,9 +14,13 @@
 # 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
 
@@ -46,3 +50,6 @@
 # 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[0x] = "/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")
Index: llvm/lib/MC/MCContext.cpp
===
--- llvm/lib/MC/MCContext.cpp
+++ llvm/lib/MC/MCContext.cpp
@@ -881,11 +881,11 @@
 
 void MCContext::addDebugPrefixMapEntry(const std::string ,
const std::string ) {
-  DebugPrefixMap.insert(std::make_pair(From, To));
+  DebugPrefixMap.emplace_back(From, To);
 }
 
 void MCContext::remapDebugPath(SmallVectorImpl ) {
-  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;
 }
Index: llvm/include/llvm/MC/MCContext.h
===
--- llvm/include/llvm/MC/MCContext.h
+++ llvm/include/llvm/MC/MCContext.h
@@ -190,7 +190,7 @@
   SmallString<128> CompilationDir;
 
   /// Prefix replacement map for source file information.
-  std::map> DebugPrefixMap;
+  SmallVector, 0> DebugPrefixMap;
 
   /// The main file name if passed in explicitly.
   std::string MainFileName;
Index: clang/tools/driver/cc1as_main.cpp
===
--- clang/tools/driver/cc1as_main.cpp
+++ clang/tools/driver/cc1as_main.cpp
@@ -97,7 +97,7 @@
   std::string DwarfDebugFlags;
   std::string DwarfDebugProducer;
   std::string DebugCompilationDir;
-  std::map DebugPrefixMap;
+  llvm::SmallVector, 0> DebugPrefixMap;
   llvm::DebugCompressionType CompressDebugSections =
   llvm::DebugCompressionType::None;
   std::string MainFileName;
@@ -275,8 +275,7 @@
 
   for (const auto  : 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
Index: clang/test/CodeGen/debug-prefix-map.c
===
--- clang/test/CodeGen/debug-prefix-map.c
+++ clang/test/CodeGen/debug-prefix-map.c
@@ -9,6 +9,10 @@
 // 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

[PATCH] D148975: -fdebug-prefix-map=: make the last win when multiple prefixes match

2023-04-25 Thread Scott Linder via Phabricator via cfe-commits 
scott.linder accepted this revision.
scott.linder added a comment.
This revision is now accepted and ready to land.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148975/new/

https://reviews.llvm.org/D148975

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148975: -fdebug-prefix-map=: make the last win when multiple prefixes match

2023-04-25 Thread Fangrui Song via Phabricator via cfe-commits 
MaskRay marked an inline comment as done.
MaskRay added inline comments.



Comment at: clang/include/clang/Basic/CodeGenOptions.h:209
 
-  std::map DebugPrefixMap;
+  llvm::SmallVector, 0> DebugPrefixMap;
   std::map CoveragePrefixMap;

scott.linder wrote:
> MaskRay wrote:
> > scott.linder wrote:
> > > What benefit does forcing allocation have? Why not use the default, or 
> > > switch to `std::vector`?
> > 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.
> 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?
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
```



Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:72-79
 CGDebugInfo::CGDebugInfo(CodeGenModule )
 : CGM(CGM), DebugKind(CGM.getCodeGenOpts().getDebugInfo()),
   DebugTypeExtRefs(CGM.getCodeGenOpts().DebugTypeExtRefs),
   DBuilder(CGM.getModule()) {
-  for (const auto  : CGM.getCodeGenOpts().DebugPrefixMap)
-DebugPrefixMap[KV.first] = KV.second;
+  for (const auto &[From, To] : CGM.getCodeGenOpts().DebugPrefixMap)
+DebugPrefixMap.emplace_back(From, To);
   CreateCompileUnit();

scott.linder wrote:
> MaskRay wrote:
> > scott.linder wrote:
> > > Can you use the member-initializer-list here?
> > No, this doesn't work. We convert a vector of `std::string` to a vector of 
> > `StringRef`.
> 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 `StringRef`s?
> 
> It seems like the original utility was just to get the `std::map` sorting 
> behavior, which we no longer need.
Thanks for the giving this more attention. Actually I found that the variable 
can be removed. Removed:)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148975/new/

https://reviews.llvm.org/D148975

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148975: -fdebug-prefix-map=: make the last win when multiple prefixes match

2023-04-25 Thread Fangrui Song via Phabricator via cfe-commits 
MaskRay updated this revision to Diff 516863.
MaskRay marked 3 inline comments as done.
MaskRay added a comment.

remove a DebugPrefixMap variable


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148975/new/

https://reviews.llvm.org/D148975

Files:
  clang/include/clang/Basic/CodeGenOptions.h
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/debug-prefix-map.c
  clang/tools/driver/cc1as_main.cpp
  llvm/include/llvm/MC/MCContext.h
  llvm/lib/MC/MCContext.cpp
  llvm/test/MC/ELF/debug-prefix-map.s

Index: llvm/test/MC/ELF/debug-prefix-map.s
===
--- llvm/test/MC/ELF/debug-prefix-map.s
+++ llvm/test/MC/ELF/debug-prefix-map.s
@@ -14,9 +14,13 @@
 # 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
 
@@ -46,3 +50,6 @@
 # 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[0x] = "/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")
Index: llvm/lib/MC/MCContext.cpp
===
--- llvm/lib/MC/MCContext.cpp
+++ llvm/lib/MC/MCContext.cpp
@@ -881,11 +881,11 @@
 
 void MCContext::addDebugPrefixMapEntry(const std::string ,
const std::string ) {
-  DebugPrefixMap.insert(std::make_pair(From, To));
+  DebugPrefixMap.emplace_back(From, To);
 }
 
 void MCContext::remapDebugPath(SmallVectorImpl ) {
-  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;
 }
Index: llvm/include/llvm/MC/MCContext.h
===
--- llvm/include/llvm/MC/MCContext.h
+++ llvm/include/llvm/MC/MCContext.h
@@ -190,7 +190,7 @@
   SmallString<128> CompilationDir;
 
   /// Prefix replacement map for source file information.
-  std::map> DebugPrefixMap;
+  SmallVector, 0> DebugPrefixMap;
 
   /// The main file name if passed in explicitly.
   std::string MainFileName;
Index: clang/tools/driver/cc1as_main.cpp
===
--- clang/tools/driver/cc1as_main.cpp
+++ clang/tools/driver/cc1as_main.cpp
@@ -97,7 +97,7 @@
   std::string DwarfDebugFlags;
   std::string DwarfDebugProducer;
   std::string DebugCompilationDir;
-  std::map DebugPrefixMap;
+  llvm::SmallVector, 0> DebugPrefixMap;
   llvm::DebugCompressionType CompressDebugSections =
   llvm::DebugCompressionType::None;
   std::string MainFileName;
@@ -275,8 +275,7 @@
 
   for (const auto  : 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
Index: clang/test/CodeGen/debug-prefix-map.c
===
--- clang/test/CodeGen/debug-prefix-map.c
+++ clang/test/CodeGen/debug-prefix-map.c
@@ -9,6 +9,10 @@
 // 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

[PATCH] D148975: -fdebug-prefix-map=: make the last win when multiple prefixes match

2023-04-25 Thread Scott Linder via Phabricator via cfe-commits 
scott.linder added inline comments.



Comment at: clang/include/clang/Basic/CodeGenOptions.h:209
 
-  std::map DebugPrefixMap;
+  llvm::SmallVector, 0> DebugPrefixMap;
   std::map CoveragePrefixMap;

MaskRay wrote:
> scott.linder wrote:
> > What benefit does forcing allocation have? Why not use the default, or 
> > switch to `std::vector`?
> 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.
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?



Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:72-79
 CGDebugInfo::CGDebugInfo(CodeGenModule )
 : CGM(CGM), DebugKind(CGM.getCodeGenOpts().getDebugInfo()),
   DebugTypeExtRefs(CGM.getCodeGenOpts().DebugTypeExtRefs),
   DBuilder(CGM.getModule()) {
-  for (const auto  : CGM.getCodeGenOpts().DebugPrefixMap)
-DebugPrefixMap[KV.first] = KV.second;
+  for (const auto &[From, To] : CGM.getCodeGenOpts().DebugPrefixMap)
+DebugPrefixMap.emplace_back(From, To);
   CreateCompileUnit();

MaskRay wrote:
> scott.linder wrote:
> > Can you use the member-initializer-list here?
> No, this doesn't work. We convert a vector of `std::string` to a vector of 
> `StringRef`.
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 `StringRef`s?

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148975/new/

https://reviews.llvm.org/D148975

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148975: -fdebug-prefix-map=: make the last win when multiple prefixes match

2023-04-24 Thread Fangrui Song via Phabricator via cfe-commits 
MaskRay marked 2 inline comments as done.
MaskRay added inline comments.



Comment at: clang/include/clang/Basic/CodeGenOptions.h:209
 
-  std::map DebugPrefixMap;
+  llvm::SmallVector, 0> DebugPrefixMap;
   std::map CoveragePrefixMap;

scott.linder wrote:
> What benefit does forcing allocation have? Why not use the default, or switch 
> to `std::vector`?
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.



Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:72-79
 CGDebugInfo::CGDebugInfo(CodeGenModule )
 : CGM(CGM), DebugKind(CGM.getCodeGenOpts().getDebugInfo()),
   DebugTypeExtRefs(CGM.getCodeGenOpts().DebugTypeExtRefs),
   DBuilder(CGM.getModule()) {
-  for (const auto  : CGM.getCodeGenOpts().DebugPrefixMap)
-DebugPrefixMap[KV.first] = KV.second;
+  for (const auto &[From, To] : CGM.getCodeGenOpts().DebugPrefixMap)
+DebugPrefixMap.emplace_back(From, To);
   CreateCompileUnit();

scott.linder wrote:
> Can you use the member-initializer-list here?
No, this doesn't work. We convert a vector of `std::string` to a vector of 
`StringRef`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148975/new/

https://reviews.llvm.org/D148975

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148975: -fdebug-prefix-map=: make the last win when multiple prefixes match

2023-04-24 Thread Scott Linder via Phabricator via cfe-commits 
scott.linder added inline comments.



Comment at: clang/include/clang/Basic/CodeGenOptions.h:209
 
-  std::map DebugPrefixMap;
+  llvm::SmallVector, 0> DebugPrefixMap;
   std::map CoveragePrefixMap;

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



Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:72-79
 CGDebugInfo::CGDebugInfo(CodeGenModule )
 : CGM(CGM), DebugKind(CGM.getCodeGenOpts().getDebugInfo()),
   DebugTypeExtRefs(CGM.getCodeGenOpts().DebugTypeExtRefs),
   DBuilder(CGM.getModule()) {
-  for (const auto  : CGM.getCodeGenOpts().DebugPrefixMap)
-DebugPrefixMap[KV.first] = KV.second;
+  for (const auto &[From, To] : CGM.getCodeGenOpts().DebugPrefixMap)
+DebugPrefixMap.emplace_back(From, To);
   CreateCompileUnit();

Can you use the member-initializer-list here?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148975/new/

https://reviews.llvm.org/D148975

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148975: -fdebug-prefix-map=: make the last win when multiple prefixes match

2023-04-21 Thread Joerg Sonnenberger via Phabricator via cfe-commits 
joerg added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148975/new/

https://reviews.llvm.org/D148975

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148975: -fdebug-prefix-map=: make the last win when multiple prefixes match

2023-04-21 Thread Fangrui Song via Phabricator via cfe-commits 
MaskRay created this revision.
MaskRay added reviewers: debug-info, dankm, gulfem, phosek.
Herald added subscribers: hiraditya, emaste.
Herald added a project: All.
MaskRay requested review of this revision.
Herald added a reviewer: jdoerfert.
Herald added subscribers: llvm-commits, cfe-commits, jplehr, sstefan1.
Herald added projects: clang, LLVM.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D148975

Files:
  clang/include/clang/Basic/CodeGenOptions.h
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/debug-prefix-map.c
  clang/tools/driver/cc1as_main.cpp
  llvm/include/llvm/MC/MCContext.h
  llvm/lib/MC/MCContext.cpp
  llvm/test/MC/ELF/debug-prefix-map.s

Index: llvm/test/MC/ELF/debug-prefix-map.s
===
--- llvm/test/MC/ELF/debug-prefix-map.s
+++ llvm/test/MC/ELF/debug-prefix-map.s
@@ -14,9 +14,13 @@
 # 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
 
@@ -46,3 +50,6 @@
 # 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[0x] = "/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")
Index: llvm/lib/MC/MCContext.cpp
===
--- llvm/lib/MC/MCContext.cpp
+++ llvm/lib/MC/MCContext.cpp
@@ -881,11 +881,11 @@
 
 void MCContext::addDebugPrefixMapEntry(const std::string ,
const std::string ) {
-  DebugPrefixMap.insert(std::make_pair(From, To));
+  DebugPrefixMap.emplace_back(From, To);
 }
 
 void MCContext::remapDebugPath(SmallVectorImpl ) {
-  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;
 }
Index: llvm/include/llvm/MC/MCContext.h
===
--- llvm/include/llvm/MC/MCContext.h
+++ llvm/include/llvm/MC/MCContext.h
@@ -190,7 +190,7 @@
   SmallString<128> CompilationDir;
 
   /// Prefix replacement map for source file information.
-  std::map> DebugPrefixMap;
+  SmallVector, 0> DebugPrefixMap;
 
   /// The main file name if passed in explicitly.
   std::string MainFileName;
Index: clang/tools/driver/cc1as_main.cpp
===
--- clang/tools/driver/cc1as_main.cpp
+++ clang/tools/driver/cc1as_main.cpp
@@ -97,7 +97,8 @@
   std::string DwarfDebugFlags;
   std::string DwarfDebugProducer;
   std::string DebugCompilationDir;
-  std::map DebugPrefixMap;
+  llvm::SmallVector, 0>
+  DebugPrefixMap;
   llvm::DebugCompressionType CompressDebugSections =
   llvm::DebugCompressionType::None;
   std::string MainFileName;
@@ -275,8 +276,7 @@
 
   for (const auto  : Args.getAllArgValues(OPT_fdebug_prefix_map_EQ)) {
 auto Split = StringRef(Arg).split('=');
-Opts.DebugPrefixMap.insert(
-{std::string(Split.first), std::string(Split.second)});
+