[PATCH] D151353: [DebugInfo] Add error-handling to DWARFAbbreviationDeclarationSet

[Felipe de Azevedo Piovezan via Phabricator via llvm-commits]

fdeazeve accepted this revision.
fdeazeve added a comment.

LGTM! Thanks for driving this



================
Comment at: llvm/unittests/DebugInfo/DWARF/DWARFDebugAbbrevTest.cpp:24
+void writeValidAbbreviationDeclarations(raw_ostream &OS, uint32_t FirstCode,
+                                        OrderKind Order) {
   encodeULEB128(FirstCode, OS);
----------------
fdeazeve wrote:
> This is likely irrelevant for the patch itself, but I missed the boat on the original review: aren't we missing the compile unit terminator for this in most tests?
> 
> 
> We have this structure:
> ```
> First code
>   DW_TAG_compile_unit
>     Has Children = true
>     Name, strp
>     0, 0   // terminates list of attributes
> 
>     DW_TAG_subprogram   // children of compile unit
>       Has children = false
>       Name, strp
>       0, 0   // terminates list of attributes
> ```
> 
> This is all the function writes. Sometimes, the callees write an extra zero, which is terminating the `DW_TAG_compile_unit` children list. (Maybe we should write that one here instead? The extra entries we write manually write in each test would simply become siblings of the TAG_compile_unit, which is fine)
> 
> 
> But even with that extra zero, we're missing an extra zero terminating the abbreviation table itself (when multiple abbreviation tables are stitched together, they are separated by this terminator, see page 207 line 15 of the D5 spec)
> 
> Anyhow, I am pointing all of this out because it's curious that the `DWARFAbbreviationDeclarationSet::extract` method seems to happily ignore the absence of this?
> 
Ahhhhhh nvm, I am making the same mistake I made months ago when I first looked into this: 

> Sometimes, the callees write an extra zero, which is terminating the DW_TAG_compile_unit children list

This is wrong. The extra zero terminating the children lists are part of the **debug info** section, not the debug abbrev section. The zero you write is indeed terminating the abbrev table itself.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151353/new/

https://reviews.llvm.org/D151353