Page MenuHomePhabricator

[ObjectYAML][DWARF] Implement the .debug_addr section.
ClosedPublic

Authored by Higuoxing on Jun 10 2020, 1:57 AM.

Details

Summary

This patch implements the .debug_addr section.

Diff Detail

Event Timeline

Higuoxing created this revision.Jun 10 2020, 1:57 AM
Herald added a project: Restricted Project. · View Herald Transcript
Higuoxing updated this revision to Diff 269764.Jun 10 2020, 2:02 AM

Remove #include <bits/stdint-uintn.h>

Higuoxing updated this revision to Diff 269767.Jun 10 2020, 2:11 AM

Remove #include "llvm/Support/YAMLTraits".

Higuoxing updated this revision to Diff 269775.Jun 10 2020, 2:53 AM

Merge D81542 to this patch.

jhenderson added inline comments.Jun 10 2020, 3:10 AM
llvm/lib/ObjectYAML/DWARFEmitter.cpp
66–71

If this function isn't dead already, we should probably aim to delete it with other changes.

77

(uint32_t)UINT32_MAX -> std::numeric_limits<uint32_t>::max()

356

I'd avoid this auto. It's not obvious what the type of DI.DebugAddr is. Same below.

368

writeVariableSizedInteger emits an assertion if you are writing values of size other than 1/2/4/8. You'll need to add an error check here for other sizes, or add some way of allowing writing of other sized integers. If you add an error check, you probably only want to trigger the check when there are one or more entries in the table, so that you can still have an invalid size without error if the table is empty.

llvm/lib/ObjectYAML/DWARFYAML.cpp
210

I would make SegmentSelectorSize optional, with a default case of 0. That is by far and away the most common case.

You could probably make AddrSize optional too, by referring to the ELF class (i.e. 64/32) if it is unspecified.

llvm/test/tools/yaml2obj/ELF/DWARF/debug-addr.yaml
2

I'm out of time today, so will do a proper review of the tests either later or tomorrow.

156

You can probably get rid of the a1) etc bits. from these comments. If you want them for cross-referencing to the output check above, I'd just make them 1), 2) etc.

grimar added inline comments.Jun 10 2020, 3:19 AM
llvm/lib/ObjectYAML/DWARFEmitter.cpp
73

I'd replace Format with Is64Bit and put it after IsLittleEndian.

81

It can be

if (Is64Bit)
  writeInteger((uint32_t)UINT32_MAX, OS, IsLittleEndian);
writeVariableSizedInteger(Length, Is64Bit ? 8 : 4, OS, IsLittleEndian);
356

Please do not use auto when the type us not obvious.

360

I'd simplify:

(uint64_t)*AddrTableEntry.Length : /*2 (version) + 1 (address size) + 1 (segment selector size) =*/ 4 +
369

no auto please

llvm/lib/ObjectYAML/DWARFYAML.cpp
206

Hmmm. I guess DWARF64 would be more common?

208

Should it be required? I guess we will be unable to change this value in the future here, because otherwise
our tests that do not specify it explicitly might start to fail. So I am not sure that we want a default value of 5 (or any default value).

grimar added inline comments.Jun 10 2020, 3:24 AM
llvm/lib/ObjectYAML/DWARFEmitter.cpp
77

Or, perhaps:

writeVariableSizedInteger(UINT32_MAX, 4, OS, IsLittleEndian);

(A bit shorter)

jhenderson added inline comments.Jun 10 2020, 3:55 AM
llvm/lib/ObjectYAML/DWARFEmitter.cpp
73

Personally, I prefer explicitly using the Format here, if I'm honest. I think it's clearer than a boolean.

llvm/lib/ObjectYAML/DWARFYAML.cpp
206

You'd guess wrong :)

DWARF64 generation is only just starting to be supported in MC. DWARF32 is the "standard" format, and being more compact, is used, unless the DWARF section is too big to fit in the 32-bit format. That is very unlikely to ever happen in a current real-world case.

Higuoxing marked 11 inline comments as done.

Address comments.

Higuoxing added inline comments.Jun 10 2020, 11:09 PM
llvm/lib/ObjectYAML/DWARFEmitter.cpp
66–71

Yes, it isn't dead already. I will propose to delete it in another patch.

73

Actually I want a variable like Is64bitTarget or Is64bitAddress in the DWARFYAML::Data structure like IsLittleEndian. With this, yaml2elf will be able to infer the AddrSize and SegmentSelectorSize below.

368

I want to do the error check in writeVariableSizedInteger in another patch.

Error Err = Error::success();
cantFail(Err);
for (const SegAddrPair& Pair : SegAddrPairs) {
  if (Seg.Size != 0) {
    Err = writeVariableSizedInteger();
    if (Err)
      return std::move(Err);
  }
  if (Addr.Size != 0) {
    Err = writeVariableSizedInteger();
    if (Err)
      return std::move(Err);
  }
}

What do you think of it?

llvm/lib/ObjectYAML/DWARFYAML.cpp
208

.debug_addr is new in DWARFv5, I am not sure if the version will be changed in DWARFv6? So I take your advice and change it to mapRequired() here :-)

210

I will make SegmentSelectorSize be 0 by default. As for the AddrSize, we have to add another field Is64bitTarget in the DWARFYAML::Data structure, I will make it optional in a separate patch.

MaskRay added inline comments.Jun 10 2020, 11:32 PM
llvm/test/tools/yaml2obj/ELF/DWARF/debug-addr.yaml
26

The ASCII art appears to be too verbose.

Looks mostly fine to me. How are you ensuring your section data is actually correct? Are you just verifying it against the specification/using llvm-dwarfdump/something else?

llvm/lib/ObjectYAML/DWARFEmitter.cpp
368

Seems reasonable, and similar to what we did with .debug_ranges Offset before.

llvm/lib/ObjectYAML/DWARFYAML.cpp
208

Yeah, I'm inclined to be cautious on this one. We can discuss it with more developers in the future. The conclusion may be that having to explicitly specify the DWARF version in individual tables is annoying. Perhaps there could be a top-level DefaultVersion entry in the DWARF tag that says what version to assume when generating tables, if otherwise unspecified.

210

I still feel like there should be a way to use the file header data to infer the address, rather than needing an explicit field, but it's fine to wait to a subsequent patch for that.

llvm/test/tools/yaml2obj/ELF/DWARF/debug-addr.yaml
26

Do you have a better suggestion for how to illustrate the section data without relying on llvm-dwarfdump (which in turn would be relying on yaml2obj in the future, and thus be circular logic)?

This is the same approach as has been taken in the other tests in this directory.

156

8 -> 4

235

Up to you, but I think it would make things a little less verbose in this test if you just left these all to be DWARF32, and add additional minimal test cases just showing the different behaviour for different DWARF formats, since the format only impacts the header fields.

244

You need a test case each for missing the version and address size fields.

Higuoxing updated this revision to Diff 270301.Jun 11 2020, 9:33 PM
Higuoxing marked 5 inline comments as done.

Address comments.

Looks mostly fine to me. How are you ensuring your section data is actually correct? Are you just verifying it against the specification/using llvm-dwarfdump/something else?

Yes, I verify it using llvm-dwarfdump except segment. llvm-dwarfdump cannot parse segment in the .debug_addr section.

llvm/test/tools/yaml2obj/ELF/DWARF/debug-addr.yaml
244

Version is a required field, do we still need to test it here?

jhenderson added inline comments.Jun 15 2020, 2:05 AM
llvm/test/tools/yaml2obj/ELF/DWARF/debug-addr.yaml
156

This comment is a bit stale, since on first glance, it looked like "are you missing the address?" before I realised that it's derived by default.

It might be best to update this comment to say something like "Show that the address size is derived from the file header." possibly in addition to other things, as you think are best. Same sort of comment below.

244

Right, but how do you show that Version is required? You need a test showing that it is, i.e. that you get an error message if the Version is unspecified.

Higuoxing updated this revision to Diff 270699.Jun 15 2020, 3:18 AM
Higuoxing marked 3 inline comments as done.

Address comments.

  • Add one test case (g) for missing 'Version'.
  • Add other common test cases.
llvm/test/tools/yaml2obj/ELF/DWARF/debug-addr.yaml
244

I added one test case (g) for missing 'Version' :-)

Higuoxing updated this revision to Diff 270702.Jun 15 2020, 3:21 AM

Remove one blank line in test cases.

jhenderson accepted this revision.Jun 15 2020, 3:57 AM

LGTM, but please wait for others to confirm.

This revision is now accepted and ready to land.Jun 15 2020, 3:57 AM
grimar accepted this revision.Jun 15 2020, 4:24 AM

LGTM

llvm/lib/ObjectYAML/DWARFYAML.cpp
206

Ah, right. I think I read it as object format and not as dwarf format.

Harbormaster completed remote builds in B60279: Diff 270702.
This revision was automatically updated to reflect the committed changes.