[PATCH] D59515: Prevent duplicate files in debug line header in dwarf 5.
Paul Robinson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Mar 21 07:45:39 PDT 2019
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
More information about the llvm-commits
mailing list