[PATCH] D87179: Fix debug_abbrev emitter to only assign table id once

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/tools/obj2yaml/dwarf2yaml.cpp:27-28
     for (auto AbbrvDeclSet : *AbbrevSetPtr) {
+      if (AbbrvDeclSet.second.begin() == AbbrvDeclSet.second.end())
+        continue;
       Y.DebugAbbrev.emplace_back();
----------------
aadsm wrote:
> jhenderson wrote:
> > aadsm wrote:
> > > jhenderson wrote:
> > > > Higuoxing wrote:
> > > > > Do we really need this check here? I think it will prevent us from dumping an empty abbrev table.
> > > > > 
> > > > > ```
> > > > > debug_abbrev:
> > > > >     - ID: 0
> > > > >       Table:
> > > > >         - Code:           1
> > > > >           Tag:            DW_TAG_compile_unit
> > > > >           Children:       DW_CHILDREN_yes
> > > > >           Attributes:
> > > > >             - Attribute:  DW_AT_producer
> > > > >               Form:       DW_FORM_strp
> > > > >     - ID: 1   ## This table will not be dumped.
> > > > > ```
> > > > Could we get a test case for this situation, please? Either separately or as part of this patch, I don't mind.
> > > That is true. I didn't realize there should always be an empty table at the end of the debug_abbrev. I'll update then.
> > I don't think there always needs to be an empty table at the end of debug_abbrev (IIRC, there is a null byte added at the end of the abbrev table, but that's not a table itself), but I think we want to allow a user to have one, if they want.
> oh, but that's exactly what will happen if the `begin() == end()` check is not there since this code optimistically creates a table before checking if it reached the last item in the iteration or not. That's the reason I added it, maybe I misunderstood @Higuoxing's comment.
Section 7.5.3 of the DWARF v4 standard discusses the format of the .debug_abbrev section (I haven't checked, but I believe it's the same for other standards). In particular there is a statement that says "The abbreviations for a given compilation unit end with an entry consisting of a 0 byte for the abbreviation code." An empty abbrev table would consist of just that 0 byte, and nothing else. So a .debug_abbrev section with the content `{0, 0, 0, 0}` would actually represent 4 separate empty tables.

We want to allow a user the ability to add an empty table. The format would be:
```
debug_abbrev:
    - ID: 42 ## This should be an empty table, with explicit ID.
    - Table: [] ## Also an empty table, with implicit ID.
    - ID: 100
      Table: [] ## Also an empty table, with explicit ID.
```
I believe all three should produce identical output (i.e. a single 0 byte), resulting in a 3-byte debug_abbrev section.

As far as I can tell, your patch looks good here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87179