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

[James Henderson via Phabricator via llvm-commits]

jhenderson 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,
----------------
Higuoxing wrote:
> 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.
> 
> 
My instinct is to do nothing at all for now. If people find it causing problems, it's functionality that could be added later. I wouldn't bother with the warning - in most cases, people will never see the warning, due to yaml2obj being run as part of lit tests.


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-str-offsets.yaml:46-47
+    - Offsets:
+        - 0x00001234
+        - 0x00005678
+    - Format: DWARF64
----------------
I meant something like `0x12345678` and `0x87654321`, i.e. so that all the bytes contain non-zero.


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-str-offsets.yaml:121
+
+## e) Test that the .debug_str_offsets section is omitted if the "debug_str_offsets" is empty.
+
----------------
Hmmm... I would expect the section header to be emitted, but for the section to be empty in this context.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83853