[PATCH] D59515: Prevent duplicate files in debug line header in dwarf 5.

[Paul Robinson via Phabricator via llvm-commits]

probinson added a comment.

Thanks for working on this!  I knew I had left duplications behind, but the task to take care of them hadn't ever gotten to the top of the list.
Mostly style things to start with.



================
Comment at: llvm/include/llvm/MC/MCContext.h:528
     const MCDwarfLineTable &getMCDwarfLineTable(unsigned CUID) const {
-      auto I = MCDwarfLineTablesCUMap.find(CUID);
+      const auto &I = MCDwarfLineTablesCUMap.find(CUID);
       assert(I != MCDwarfLineTablesCUMap.end());
----------------
You could constify as a separate NFC preliminary.


================
Comment at: llvm/include/llvm/MC/MCDwarf.h:221
   bool HasAnyMD5 = false;
+  unsigned DwarfVersion = 4;
 
----------------
Because the only constructor requires a DwarfVersion parameter, you don't need to initialize here?


================
Comment at: llvm/include/llvm/MC/MCDwarf.h:224
 public:
-  MCDwarfLineTableHeader() = default;
+  MCDwarfLineTableHeader(unsigned DwarfVersion) : DwarfVersion(DwarfVersion) {}
 
----------------
`explicit` ?  Don't want this to be a converting constructor.


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:1914
   // directly, the label is the only work required here.
-  auto &Tables = getContext().getMCDwarfLineTables();
+  const auto &Tables = getContext().getMCDwarfLineTables();
   if (!Tables.empty()) {
----------------
You could constify as a separate NFC preliminary.


================
Comment at: llvm/lib/MC/MCDwarf.cpp:545
 
+bool IsRootFile(const MCDwarfFile &RootFile, StringRef &Directory,
+                StringRef &FileName, MD5::MD5Result *Checksum) {
----------------
Current style is lowerCamelCase for methods, so `isRootFile` here.
I know MC is inconsistent on this point, but new code should use the new style.


================
Comment at: llvm/test/CodeGen/AMDGPU/flat-error-unsupported-gpu-hsa.ll:11
+; ERROR: LLVM ERROR: Cannot select:
+; ERROR-SAME:  i32,ch = load<(volatile load 4 from %ir.flat.ptr.load)>
 ; HSA-DEFAULT: flat_load_dword
----------------
This looks unrelated, please rebase.


================
Comment at: llvm/test/tools/llvm-objdump/X86/function-sections-line-numbers.s:33
 	.file	0 "/home/avl" "test.cpp" md5 0xefae234cc05b45384d782316d3a5d338
 	.file	1 "test.cpp" md5 0xefae234cc05b45384d782316d3a5d338
+	.loc	0 1 0                   # test.cpp:1:0
----------------
Curious why this `.file 1` is still present, it looks redundant now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59515