[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/test/ObjectYAML/MachO/DWARF-debug_info.yaml:683
 
-# RUN: yaml2obj --docnum=3 %s | obj2yaml | FileCheck %s --check-prefix=MULTI-TABLES
+# RUN: yaml2obj --docnum=3 %s | obj2yaml | FileCheck %s --check-prefix=MULTI-TABLES --dump-input=always
 
----------------
aadsm wrote:
> jhenderson wrote:
> > Remove `--dump-input` from the FileCheck command.
> > 
> > I actually have rarely used the option myself - you can achieve the goal of getting FileCheck to dump the input by having no check prefixes that match the prefix being looked for, which is often a good starting point.
> Not sure if I understand what you mean with "having no check prefixes that match the prefix being looked for," but I'll remove this! I added for debugging to see where the mismatch was but forgot to remove it.
Trivial example of what I meant:
```
# test.test
# RUN: echo foo | FileCheck %s
# eof
```
When at least run with lit -v, you get the output of `echo foo` printed by FileCheck as part of the error for not having any matching `CHECK` directives. Same applies if you e.g. do `--check-prefix=BAZ` and include no `BAZ:` lines etc. I often don't write my `CHECK` lines in my test until after running the test once, which allows me to then copy this output into the test for finer tuning.


================
Comment at: llvm/test/ObjectYAML/MachO/DWARF-debug_info.yaml:858
       AbbrevTableID: 1
-      AbbrOffset:    16
+      AbbrOffset:    23
       Entries:
----------------
Higuoxing wrote:
> We can simply remove this line since yaml2obj is able to calculate the value of `debug_abbrev_offset` for us.
Looks like this hasn't been addressed?


================
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:
> > 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87179