Page MenuHomePhabricator

Add .debug_ranges support to the DWARF YAML.
ClosedPublic

Authored by clayborg on Apr 23 2020, 7:30 PM.

Details

Summary

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.

Diff Detail

Event Timeline

clayborg created this revision.Apr 23 2020, 7:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 23 2020, 7:30 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
labath edited reviewers, added: jhenderson; removed: labath.Apr 24 2020, 5:14 AM
labath added a subscriber: labath.

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.

clayborg marked an inline comment as done.Apr 26 2020, 1:54 PM
clayborg added inline comments.
llvm/test/tools/obj2yaml/macho-DWARF-debug-ranges.yaml
32

You mean check in an object file instead of using yaml?

clayborg updated this revision to Diff 260183.Apr 26 2020, 2:03 PM

Updated things labath commented on by renaming Descriptors to Entries.

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.

labath added inline comments.Apr 27 2020, 5:43 AM
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
aprantl added inline comments.Apr 27 2020, 2:44 PM
llvm/include/llvm/ObjectYAML/DWARFYAML.h
79

Doxygen comment that says which entries/section this corresponds?

clayborg marked 3 inline comments as done.Apr 27 2020, 6:58 PM

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.

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

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.

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.

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.

clayborg updated this revision to Diff 260721.Apr 28 2020, 12:09 PM
  • 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
clayborg marked 13 inline comments as done.Apr 28 2020, 12:11 PM

Everything should be updated as requested. Let me know if there is anything else needed

clayborg updated this revision to Diff 261108.Apr 29 2020, 6:17 PM

Removed whitespace that caused clang format violation.

clayborg updated this revision to Diff 261109.Apr 29 2020, 6:21 PM

Remove whitespace.

Hi, I am not qualified to review, I leave some comments below :)

llvm/test/tools/obj2yaml/macho-DWARF-debug-ranges.yaml
97

Since the content of __debug_abbrev is provided by debug_abbrev in DWARF entry below. I think the content field can be removed?

110

Ditto.

123

Ditto.

159–164

Are these empty strings used as placeholders? If not, I think they can be removed?

Higuoxing added inline comments.Apr 29 2020, 8:19 PM
llvm/test/tools/obj2yaml/macho-DWARF-debug-ranges.yaml
136

__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
clayborg marked 4 inline comments as done.Apr 30 2020, 12:42 AM
clayborg added inline comments.
llvm/test/tools/obj2yaml/macho-DWARF-debug-ranges.yaml
97

This is how obj2yaml converted my original mach-o file so I can't remove it.

110

This is how obj2yaml converted my original mach-o file so I can't remove it.

123

This is how obj2yaml converted my original mach-o file so I can't remove it.

136

This is how obj2yaml converted my original mach-o file so I can't remove it.

jhenderson added inline comments.Apr 30 2020, 1:03 AM
llvm/include/llvm/ObjectYAML/DWARFYAML.h
75

Nit: missing full stop.

llvm/test/tools/obj2yaml/macho-DWARF-debug-ranges.yaml
136

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.

Higuoxing added inline comments.Apr 30 2020, 1:06 AM
llvm/test/tools/obj2yaml/macho-DWARF-debug-ranges.yaml
97

Oh, got it, thank you.
I don't know if it's applied here, some MachO-DWARF related tests are placed in llvm/test/ObjectYAML/MachO

clayborg updated this revision to Diff 261320.Apr 30 2020, 12:36 PM
clayborg marked 3 inline comments as done.

Fixed inline comments and a clang format issue.

llvm/test/tools/obj2yaml/macho-DWARF-debug-ranges.yaml
164

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:

  • I modified my DWARF generator to emit two different compile units with different address sizes but obj2yaml won't load the DWARF as the DWARF parser asserts when it tries to make the YAML by loading the DWARF
  • 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

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.

jhenderson accepted this revision.May 1 2020, 2:13 AM

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?

This revision is now accepted and ready to land.May 1 2020, 2:13 AM
dblaikie added inline comments.May 1 2020, 8:24 AM
llvm/tools/obj2yaml/dwarf2yaml.cpp
95–96

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?

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.

This revision was automatically updated to reflect the committed changes.
mtrofin added inline comments.
llvm/include/llvm/ObjectYAML/DWARFYAML.h
162

This seems to cause bot failure:

http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-ubuntu/builds/5553/steps/build-unified-tree/logs/stdio

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