This is an archive of the discontinued LLVM Phabricator instance.

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

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



This patch helps make the Code optional in abbreviations table.

Diff Detail

Event Timeline

Higuoxing created this revision.Jun 15 2020, 1:14 AM
grimar added inline comments.Jun 15 2020, 2:18 AM

Looks unformatted.

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


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


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.


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

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.


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

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.