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

[António Afonso via Phabricator via llvm-commits]

aadsm 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
 
----------------
jhenderson wrote:
> 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.
oh I see, that won't help me because my use case is not building the test. It's once the test is up and running a modification makes it fail and I need to know which lines are failing.


================
Comment at: llvm/test/ObjectYAML/MachO/DWARF-debug_info.yaml:858
       AbbrevTableID: 1
-      AbbrOffset:    16
+      AbbrOffset:    23
       Entries:
----------------
jhenderson wrote:
> 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?
yes, I missed this, will address before landing.


================
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();
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87179