[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/tools/obj2yaml/MachO/debug-abbrev.yaml:4
+#      ABBREV: DWARF:
+# ABBREV-NEXT:   debug_abbrev:
+# ABBREV-NEXT:     - ID:              0
----------------
Higuoxing wrote:
> aadsm wrote:
> > This test could be better if I added debug_infos and made sure it was pointing to table 0 as well. It would be a pain for me to make all the sizes and offsets match though. Is there an easy way to achieve this? How were the other tests created?
> Yeah, creating Mach-O tests are painful. I create them by manually assigning offset and size as well. When handcrafting minimal Mach-O test cases, I usually focus on the following fields.
> 
> * The ncmds and sizeofcmds in the FileHeader: For DWARF tests, ncmds should be 1 since there is only one load command (LC_SEGMENT_64). sizeofcmds should be greater than or equal to the size of load commands and less than or equal to the size of object file minus the size of mach_header_64 (actual_file_size - sizeof(mach_header_64)). In this case, sizeofcmds should be greater than or equal to 8 since we only have one load command and sizeof(load_command) is 8.
> 
> * The cmdsize field of load commands: cmdsize must be a multiple of 8 in 64-bit object files or a multiple of 4 in 32-bit object files. When there are 1 section in the load command, 152 would cover most cases. When there are 2 sections in the load command, 232 would cover most cases. I didn't investigate it very much.
> 
> * The size field of sections: Firstly, we can use yaml2elf create a ELF object file with DWARF sections and use `readelf -S` to check the debug sections' size. The section size in Mach-O is similar to the size in ELF.
> 
> * The offset field of sections: I didn't investigate it as well. But 528 is usually fine for the first section.
> 
> I think we can improve it in the future 😂
oh wow :D, sound good, I'll do that! if many more tests are needed might be worthwhile writing a little python script to automate the math.


================
Comment at: llvm/tools/obj2yaml/dwarf2yaml.cpp:42-46
+        if (!Y.DebugAbbrev.back().ID.hasValue())
+          Y.DebugAbbrev.back().ID = AbbrevTableID++;
         Y.DebugAbbrev.back().Table.push_back(Abbrv);
       }
     }
----------------
Higuoxing wrote:
> It looks that `Y.DebugAbbrev.back().ID = AbbrevTableID++;` should be put in the outer loop.
ah yeah of course, not sure why I added an if instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87179