Page MenuHomePhabricator

[DWARFYAML][debug_abbrev] Make the abbreviation code optional.
ClosedPublic

Authored by Higuoxing on Jun 15 2020, 1:14 AM.

Details

Summary

This patch helps make the Code optional in abbreviations table.

Diff Detail

Event Timeline

Higuoxing created this revision.Jun 15 2020, 1:14 AM
Herald added a project: Restricted Project. · View Herald Transcript
grimar added inline comments.Jun 15 2020, 2:18 AM
llvm/lib/ObjectYAML/DWARFEmitter.cpp
100

Looks unformatted.

It is also more common to write in the following way I think:

(uint64_t)*AbbrevDecl.Code

(i.e. no brackets around the AbbrevDecl.Code)

llvm/test/tools/yaml2obj/ELF/DWARF/debug-abbrev.yaml
272

Could you add a one more DW_TAG_subprogram with the same Code and one more DW_TAG_subprogram after it
somewhere here to demostrate the behavior in this case?

Higuoxing planned changes to this revision.Jun 15 2020, 2:18 AM

Seems that build bots are not happy with this change. I'll update it later.

jhenderson added a comment.EditedJun 15 2020, 2:19 AM

The premerge bot is complaining about several failing tests in ObjectYAML. You should probably investigate those.

Edit to add: oh I see you've spotted that already.

llvm/lib/ObjectYAML/DWARFEmitter.cpp
100

Something looks off in this formatting. Did you run clang-format and/or mean (uint64_t)(*AbbrevDecl.Code) perhaps?

Higuoxing updated this revision to Diff 271271.Jun 16 2020, 9:20 PM
Higuoxing marked 3 inline comments as done.

Address comments.

jhenderson added inline comments.Jun 17 2020, 1:04 AM
llvm/include/llvm/ObjectYAML/DWARFYAML.h
55

I would suggest you keep this as a Hex64 in this patch, and change it to a uint64_t in a separate patch, although I'm not entirely convinced either way whether it should be done at all. Perhaps @grimar has some thoughts?

Higuoxing updated this revision to Diff 271298.Jun 17 2020, 1:38 AM
Higuoxing marked 2 inline comments as done.

Address comment.

llvm/include/llvm/ObjectYAML/DWARFYAML.h
55

Sure, I will leave it as yaml::Hex64 in this patch. I think dumping abbreviation codes in a decimal format has better readability than dumping it in a hexadecimal format. But I'm convincible here :)

grimar added inline comments.Jun 17 2020, 2:17 AM
llvm/include/llvm/ObjectYAML/DWARFYAML.h
55

I'd change to uint64_t, but separatelly. I think that abbreviation codes in a decimal format would probably look better, because it is just an index?

This revision is now accepted and ready to land.Jun 17 2020, 2:32 AM
This revision was automatically updated to reflect the committed changes.