This allows DIEs with DW_AT_ranges to be encoded and decoded _and_ actually have their address ranges be included instead of having DW_AT_ranges with a section offset value for a section that doesn't exist.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
I don't think I am a good person to review this, as so far, I haven't found the current dwarfyaml representation to be particularly useful. I've left some comments inline.
llvm/include/llvm/ObjectYAML/DWARFYAML.h | ||
---|---|---|
75–76 | The comments aren't really correct, because the range lists can also contain base address selection entries (low = 0xffff, high = base), and subsequent entries are then relative to that. | |
llvm/lib/ObjectYAML/DWARFEmitter.cpp | ||
121 | What if Ranges.Offset is strictly less than CurrOffset? Should that assert or something? | |
llvm/lib/ObjectYAML/DWARFYAML.cpp | ||
88 | DWARF4 spec calls these "entries". You probably copied these from the "descriptors" for aranges, but it looks like there is already a precedent for the term "Entries" in the "Pubsection" code a couple of lines lower. | |
llvm/test/tools/obj2yaml/macho-DWARF-debug-ranges.yaml | ||
7–32 | As you have spelled this out anyway, maybe you could just make this into an actual check (by running llvm-dwarfdump on the intermediate object file). | |
255–295 | It doesn't look like the line section is needed to test this functionality. |
I've not taken a look at this yet, but it's worth noting that we've got a GSOC proposal this year by @Higuoxing, which, if it gets accepted will aim to write a lot more of this sort of thing.
llvm/test/tools/obj2yaml/macho-DWARF-debug-ranges.yaml | ||
---|---|---|
32 | You mean check in an object file instead of using yaml? |
Does this patch add support in yaml2obj for debug_ranges (I get a bit lost as to what point this happens)? If it is newly supported, it should be tested in a yaml2obj test too.
llvm/test/tools/obj2yaml/macho-DWARF-debug-ranges.yaml | ||
---|---|---|
2 | Something I'm trying to encourage where possible is to use '##' for comments in lit tests, as it makes the comments stand out from the functional parts of the test (lit and FileCheck run lines). You'll see that practice in many newer tests for obj2yaml/yaml2obj etc. | |
14 | It might be a good idea to add a test case for a base selection entry, with the first "address" being 0xffffffffffffffff, and then confirm that obj2yaml/yaml2obj can handle it and subsequent entries (or emit a sensible error if not). | |
54 | Nit: unnecessary blank line. | |
llvm/tools/obj2yaml/dwarf2yaml.cpp | ||
87 | Nit: delete blank line. | |
88–89 | Rather than this assumption, maybe it would be better to emit an error in this patch, if the sizes differ. | |
107 | clang-format is complaining in a few places in this file. Please fix before committing. |
llvm/test/tools/obj2yaml/macho-DWARF-debug-ranges.yaml | ||
---|---|---|
32 | What I meant was something like: RUN: yaml2obj %s > %t RUN: llvm-dwarfdump %t | FileCheck --check-prefix=DWARF RUN: obj2yaml %t | FileCheck --check-prefix=YAML DWARF: check dwarfdump output YAML: check yaml output |
llvm/include/llvm/ObjectYAML/DWARFYAML.h | ||
---|---|---|
79 | Doxygen comment that says which entries/section this corresponds? |
Already take care of with the test I added:
# RUN: yaml2obj %s | obj2yaml | FileCheck %s
I was looking around to see how the existing DWARF yaml code was tested and didn't find any great examples that split things up this way.
llvm/test/tools/obj2yaml/macho-DWARF-debug-ranges.yaml | ||
---|---|---|
2 | will do | |
14 | good idea | |
33 | Gotcha, I can do that |
That's not exactly testing what I'd expect. The obj2yaml output emits an Entries tag with a series of Offsets, but the input to yaml2obj in the test just uses raw content. I'd expect to see a yaml2obj input that consumes an Entries array, effectively showing that what obj2yaml emits can be consumed by yaml2obj.
- Updated yaml test case to encoded DW_AT_ranges with a base address entry to verify this works
- Change test case to save yaml to a tmp object file and then run llvm-dwarfdump and obj2yaml on it
- fixed all other issues mentioned in comments
Everything should be updated as requested. Let me know if there is anything else needed
Hi, I am not qualified to review, I leave some comments below :)
llvm/test/tools/obj2yaml/macho-DWARF-debug-ranges.yaml | ||
---|---|---|
96 | Since the content of __debug_abbrev is provided by debug_abbrev in DWARF entry below. I think the content field can be removed? | |
109 | Ditto. | |
122 | Ditto. | |
158–163 | Are these empty strings used as placeholders? If not, I think they can be removed? |
llvm/test/tools/obj2yaml/macho-DWARF-debug-ranges.yaml | ||
---|---|---|
135 | __debug_str's content can also be placed in DWARF entry below, which is easier to read and write. DWARF: debug_str: - '' - /tmp/main.c - stripped - stripped2 - main |
llvm/test/tools/obj2yaml/macho-DWARF-debug-ranges.yaml | ||
---|---|---|
96 | This is how obj2yaml converted my original mach-o file so I can't remove it. | |
109 | This is how obj2yaml converted my original mach-o file so I can't remove it. | |
122 | This is how obj2yaml converted my original mach-o file so I can't remove it. | |
135 | This is how obj2yaml converted my original mach-o file so I can't remove it. |
llvm/include/llvm/ObjectYAML/DWARFYAML.h | ||
---|---|---|
75 | Nit: missing full stop. | |
llvm/test/tools/obj2yaml/macho-DWARF-debug-ranges.yaml | ||
135 | It sounds like obj2yaml for Mach-O (and COFF for that matter) is in dire need of cleaning up, so that it is more useful for minimal inputs. For example, as @Higuoxing's and my own confusion have pointed out, it doesn't seem right that there is both section content and DWARF data describing the same thing. Which is the source of truth after all? Not an issue for this patch of course. | |
llvm/tools/obj2yaml/dwarf2yaml.cpp | ||
95–96 | Test case? | |
110 | The linter is still complaining about clang-format on this line. |
llvm/test/tools/obj2yaml/macho-DWARF-debug-ranges.yaml | ||
---|---|---|
96 | Oh, got it, thank you. |
Fixed inline comments and a clang format issue.
llvm/test/tools/obj2yaml/macho-DWARF-debug-ranges.yaml | ||
---|---|---|
163 | Seems like another bug in the existing mach-o YAML stuff out of the scope of this patch. Don't want to remove in case it causes problems with the current existing code as this was created by the current obj2yaml. | |
llvm/tools/obj2yaml/dwarf2yaml.cpp | ||
95–96 | I tried to make a test case and ran into multiple issues:
This really never happens in reality. I had this as an assert before, but you requested the error. Since the DWARF parsing code already asserts on this issue, I would be fine if we did too here? | |
110 | I kept seeing this in reverse thinking it didn't want the space... After looking around it seems that a space _is_ supposed to be added. |
LGTM, with one small request.
llvm/tools/obj2yaml/dwarf2yaml.cpp | ||
---|---|---|
95–96 | Sounds to me like the DWARF parser asserting is a bug in the code, but that's outside the scope of this patch. We shouldn't ever be able to hit asserts, however broken the input is. Could you add a TODO noting that this error is untested due to the parser asserting when it happens? |
llvm/tools/obj2yaml/dwarf2yaml.cpp | ||
---|---|---|
95–96 |
I didn't quite follow this - could you explain that in more detail/other words? Specifically, yes, it would be difficult/annoying to hand-craft such an input, but what were the particular problems you hit when attempting to do so? |
if I hand edit the YAML, section sizes change and all of the data is off since and there are the existing yaml2obj ignores a different AddrSize
I didn't quite follow this - could you explain that in more detail/other words? Specifically, yes, it would be difficult/annoying to hand-craft such an input, but what were the particular problems you hit when attempting to do so?
I meant if I take an example file with 2 DWARF compile units and then manually try to edit the YAML to set the compile unit addr size to 8 on one of them (where they were both 4 to begin with, then the section sizes end up being wrong and other errors can occur when trying to obj2yaml the modified YAML. Very common stuff where the YAML can't be heavily modified or changed without things failing.
llvm/include/llvm/ObjectYAML/DWARFYAML.h | ||
---|---|---|
162 | This seems to cause bot failure: FAILED: lib/ObjectYAML/CMakeFiles/LLVMObjectYAML.dir/DWARFEmitter.cpp.o /usr/bin/c++ -DEXPENSIVE_CHECKS -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GLIBCXX_ASSERTIONS -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Ilib/ObjectYAML -I/home/buildbot/as-builder-4/llvm-clang-x86_64-expensive-checks-ubuntu/llvm-project/llvm/lib/ObjectYAML -Iinclude -I/home/buildbot/as-builder-4/llvm-clang-x86_64-expensive-checks-ubuntu/llvm-project/llvm/include -U_GLIBCXX_DEBUG -fPIC -fvisibility-inlines-hidden -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wno-comment -fdiagnostics-color -g -fno-exceptions -fno-rtti -std=c++14 -MD -MT lib/ObjectYAML/CMakeFiles/LLVMObjectYAML.dir/DWARFEmitter.cpp.o -MF lib/ObjectYAML/CMakeFiles/LLVMObjectYAML.dir/DWARFEmitter.cpp.o.d -o lib/ObjectYAML/CMakeFiles/LLVMObjectYAML.dir/DWARFEmitter.cpp.o -c /home/buildbot/as-builder-4/llvm-clang-x86_64-expensive-checks-ubuntu/llvm-project/llvm/lib/ObjectYAML/DWARFEmitter.cpp In file included from /home/buildbot/as-builder-4/llvm-clang-x86_64-expensive-checks-ubuntu/llvm-project/llvm/lib/ObjectYAML/DWARFEmitter.cpp:18:0: /home/buildbot/as-builder-4/llvm-clang-x86_64-expensive-checks-ubuntu/llvm-project/llvm/include/llvm/ObjectYAML/DWARFYAML.h:162:23: error: declaration of ‘std::vector<llvm::DWARFYAML::Ranges> llvm::DWARFYAML::Data::Ranges’ [-fpermissive] std::vector<Ranges> Ranges; ^~~~~~ /home/buildbot/as-builder-4/llvm-clang-x86_64-expensive-checks-ubuntu/llvm-project/llvm/include/llvm/ObjectYAML/DWARFYAML.h:82:8: error: changes meaning of ‘Ranges’ from ‘struct llvm::DWARFYAML::Ranges’ [-fpermissive] struct Ranges { |
Fixed buildbots with:
commit 6e73f12a641bb8fd965d28937ea3a6d1dcecc8c0 (HEAD -> master, origin/master, origin/HEAD)
Author: Greg Clayton <gclayton@fb.com>
Date: Wed May 13 22:01:57 2020 -0700
Fix buildbots errors after comitting D78782. Rename "Ranges" variables to "DebugRanges" to avoid warnings/errors on machines that have extra settings enabled. https://reviews.llvm.org/D78782
Nit: missing full stop.