[PATCH] D83853: [DWARFYAML] Implement the .debug_str_offsets section.

Wed Jul 15 01:35:37 PDT 2020

Higuoxing added inline comments.

================
Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:434
+    for (uint64_t Offset : Table.Offsets) {
+      cantFail(writeVariableSizedInteger(Offset,
+                                         Table.Format == dwarf::DWARF64 ? 8 : 4,
----------------
jhenderson wrote:
> A thought I've just had: what do you think should happen if a user specifies a 64-bit-using value for DWARF32? This may apply in one or two other places too.
> 
> I don't have any particular opinions here. Just wondering if you've considered it at all.
> Just wondering if you've considered it at all.

Yes, I've considered it before.

At first, It seemed to me that the format should be inferred from the content. When the length is greater than `UINT32_MAX` or the offset is greater than `UINT32_MAX`, the DWARF64 should be specified automatically. However, this approach makes our implementation complicated. Besides, DWARF32 is the most commonly used format. I think it's not a good approach.

Another approach is to check whether the integer is able to be encoded using the given size and warn the user about it.

================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-str-offsets.yaml:120
+  debug_str_offsets:
+    - Offsets: []
+
----------------
jhenderson wrote:
> Perhaps it would be worth a test case for there being no tables at all, i.e:
> ```
> DWARF:
>   debug_str_offsets: []
> ```
> 
> I'd expect an empty section to be emitted in this case.
> 
> This might also be a missing test case in some (most?) of the other sections.
Yes, this test is missing in other tests. I'll add them. Thanks!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83853