This patch implements the .debug_addr section.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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() | |
357 | I'd avoid this auto. It's not obvious what the type of DI.DebugAddr is. Same below. | |
369 | 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 | ||
209 | 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. |
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); | |
357 | Please do not use auto when the type us not obvious. | |
361 | I'd simplify: (uint64_t)*AddrTableEntry.Length : /*2 (version) + 1 (address size) + 1 (segment selector size) =*/ 4 + | |
370 | no auto please | |
llvm/lib/ObjectYAML/DWARFYAML.cpp | ||
205 | Hmmm. I guess DWARF64 would be more common? | |
207 | Should it be required? I guess we will be unable to change this value in the future here, because otherwise |
llvm/lib/ObjectYAML/DWARFEmitter.cpp | ||
---|---|---|
77 | Or, perhaps: writeVariableSizedInteger(UINT32_MAX, 4, OS, IsLittleEndian); (A bit shorter) |
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 | ||
205 | 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. |
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. | |
369 | 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 | ||
207 | .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 :-) | |
209 | 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. |
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 | ||
---|---|---|
369 | Seems reasonable, and similar to what we did with .debug_ranges Offset before. | |
llvm/lib/ObjectYAML/DWARFYAML.cpp | ||
207 | 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. | |
209 | 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. |
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? |
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. |
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' :-) |
LGTM
llvm/lib/ObjectYAML/DWARFYAML.cpp | ||
---|---|---|
205 | Ah, right. I think I read it as object format and not as dwarf format. |
If this function isn't dead already, we should probably aim to delete it with other changes.