Apply -fdebug-prefix-map in reverse of command line order

[alxu]

Summary

Before this patch, it is applied in order of increasing OLD path length. This is not a useful behavior.

After this patch, it is applied based on the command line order from right to left, stopping on the first match.

[alxu]

Oh, I forgot to mention, this is the way that gcc does it. Therefore, I expect that almost everybody either doesn't care about the order, or assumes the gcc behavior.

[probinson]

Needs a test.

[probinson]

Also, please document the option in clang/docs/UsersManual.rst. It should have been added when the option was first added, but certainly with right-to-left behavior it needs a mention. Nearly all options work left-to-right, so even though it's the same as gcc, not everybody comes to clang from gcc.

[alxu]

The test syntax looked complicated, and I assumed that the documentation was terrible, like wine's, but it's actually pretty good, so I'll give it a go. I think the order is not really surprising though. It is run from right to left, but basically it means that right options override left options, which is how most of them work, e.g. -W and -Wno work based on the rightmost toggle, same with -f and -fno, etc.

[alxu]

Also, is it right to use SmallVector in one place and vector in the other? I just assumed based on the surrounding declarations, but I really have no idea.

[probinson]

Re. SmallVector versus std::vector, they are functionally similar but have different memory-allocation behaviors. SmallVector includes a vector of N elements (where N is the template parameter) so does no dynamic allocation until you have more than N elements; but it takes up that much more space as a member of a class. It's mainly intended for stack variables. Given that this particular option is used rarely, I would probably go with std::vector. The code change otherwise is pretty simple and looks fine.

As for testing, it has its quirks for sure. But I'm happy to help sort out any issues. There should be plenty of CodeGen examples with debug info in the test tree to imitate. You'll want to start with C/C++ source and use -emit-llvm to get textual IR, then use FileCheck (our fancy-pants grep) to look for the new pathname patterns.

[alxu]

I was just going to run llvm-dwarfdump.

[probinson]

In D49652#1172352, @alxu wrote:

I was just going to run llvm-dwarfdump.

Well, that's one of those quirks I mentioned. This is a Clang change, and Clang tests should exercise as little of LLVM as possible. That means you don't generate assembler or an object file, if you can possibly help it. And in this case, the path renaming should be visible in the debug-info metadata, which makes it easy enough to test by emitting LLVM IR and looking at that. It's by far the most common test idiom for changes to Clang's CodeGen layer.

Given that the prefix mapping happens in the LLVM IR, it should be no harder to emit IR and write a pattern-match than it would be for dwarfdump output. Really. Try clang -g -S -emit-llvm on some simple test file, then throw a remap option at it and see what's different.

[alxu]

my general theory is that it's better to have tests that test everything at once if possible, but I guess it's unlikely enough that the debug info path will be correct in the IR and then not correct in the generated file and that that won't be caught by LLVM tests.

[probinson]

In D49652#1172498, @alxu wrote:

my general theory is that it's better to have tests that test everything at once if possible, but I guess it's unlikely enough that the debug info path will be correct in the IR and then not correct in the generated file and that that won't be caught by LLVM tests.

The in-tree lit tests are very focused unit tests. The principle here is that a more focused test will be easier to diagnose when it fails. The LLVM project on its own really does not have much end-to-end testing and hardly any of that looks at debug info. Instead we have a bunch of vendors all running their own test suites. It's very much a tradeoff, and I personally view the in-tree lit tests as barely more than basic smoke tests. But it's how the project works, and so new in-tree tests need to be consistent with that approach.

include/clang/Frontend/CodeGenOptions.h

public:
 
   /// The string to embed in debug information as the current working directory.
   std::string DebugCompilationDir;
 
   /// The string to embed in the debug information for the compile unit, if
   /// non-empty.
   std::string DwarfDebugFlags;
 
-  std::map<std::string, std::string> DebugPrefixMap;
+  std::vector<std::pair<std::string, std::string>> DebugPrefixMap;
 
   /// The ABI to use for passing floating point arguments.
   std::string FloatABI;
 
   /// The floating-point denormal mode to use.
   std::string FPDenormalMode;
 
   /// The float precision limit to use, if non-empty.

lib/CodeGen/CGDebugInfo.h

#include "clang/Basic/OpenCLImageTypes.def"
   llvm::DIType *OCLClkEventDITy = nullptr;
   llvm::DIType *OCLQueueDITy = nullptr;
   llvm::DIType *OCLNDRangeDITy = nullptr;
   llvm::DIType *OCLReserveIDDITy = nullptr;
 
   /// Cache of previously constructed Types.
   llvm::DenseMap<const void *, llvm::TrackingMDRef> TypeCache;
 
-  llvm::SmallDenseMap<llvm::StringRef, llvm::StringRef> DebugPrefixMap;
+  llvm::SmallVector<std::pair<llvm::StringRef, llvm::StringRef>, 4> DebugPrefixMap;
 
   /// Cache that maps VLA types to size expressions for that type,
   /// represented by instantiated Metadata nodes.
   llvm::SmallDenseMap<QualType, llvm::Metadata *> SizeExprCache;
 
   struct ObjCInterfaceCacheEntry {
     const ObjCInterfaceType *Type;
     llvm::DIType *Decl;

lib/CodeGen/CGDebugInfo.cpp

 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;
+  for (auto C = CGM.getCodeGenOpts().DebugPrefixMap,
+            I = C.rbegin(), E = C.rend(); I != E; ++I)
+    DebugPrefixMap.push_back(std::make_pair((*I).first, (*I).second));
   CreateCompileUnit();
 }
 
 CGDebugInfo::~CGDebugInfo() {
   assert(LexicalBlockStack.empty() &&
          "Region stack mismatch, stack not empty!");
 }
 

lib/Frontend/CompilerInvocation.cpp

static bool ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args, InputKind IK,
   Opts.SplitDwarfFile = Args.getLastArgValue(OPT_split_dwarf_file);
   Opts.SplitDwarfInlining = !Args.hasArg(OPT_fno_split_dwarf_inlining);
   Opts.DebugTypeExtRefs = Args.hasArg(OPT_dwarf_ext_refs);
   Opts.DebugExplicitImport = Args.hasArg(OPT_dwarf_explicit_import);
   Opts.DebugFwdTemplateParams = Args.hasArg(OPT_debug_forward_template_params);
   Opts.EmbedSource = Args.hasArg(OPT_gembed_source);
 
   for (const auto &Arg : Args.getAllArgValues(OPT_fdebug_prefix_map_EQ))
-    Opts.DebugPrefixMap.insert(StringRef(Arg).split('='));
+    Opts.DebugPrefixMap.push_back(StringRef(Arg).split('='));
 
   if (const Arg *A =
           Args.getLastArg(OPT_emit_llvm_uselists, OPT_no_emit_llvm_uselists))
     Opts.EmitLLVMUseLists = A->getOption().getID() == OPT_emit_llvm_uselists;
 
   Opts.DisableLLVMPasses = Args.hasArg(OPT_disable_llvm_passes);
   Opts.DisableLifetimeMarkers = Args.hasArg(OPT_disable_lifetimemarkers);
   Opts.DisableO0ImplyOptNone = Args.hasArg(OPT_disable_O0_optnone);