[PATCH] D81063: [DWARFYAML][debug_aranges] Replace InitialLength by Format and Length.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/ObjectYAML/DWARFYAML.h:68
+  llvm::dwarf::DwarfFormat Format;
+  uint32_t LengthPrefix;
+  uint64_t Length;
----------------
Higuoxing wrote:
> jhenderson wrote:
> > I wouldn't bother with a length prefix. I'm not sure it's important for testing, because either the parser will read the DWARF64 marker (i.e. 0xffffffff) and treat the next 8 bytes as the rest of the length, or it won't in which case, it will treat those 4 bytes as the actual length, regardless of their value (with possible exceptions for the rest of the reserved range, but those values can be entered as the length in the YAML document anyway).
> Sorry, I'm a little bit lost here. What do you mean by saying
> 
> > "but those values can be entered as the length in the YAML document anyway"?
> 
> I'm not sure if I understand it correctly. The 12-bit initial length is valid only when the format is DWARF64 (the prefix is 0xffffffff). When the prefix is in 0xfffffff0-0xfffffffe, the DWARF parser will not interpret the last 8-byte as length?
> 
> So we don't need LengthPrefix here, right? We can test the prefix in the preserved range by crafting something like
> 
> ```
> Format: DWARF32
> Length: 0xfffffff0
> ```
Yes, exactly. The `LengthPrefix` isn't needed at all, I believe. The users options are:

```
Format: DWARF32
Length: <some value, including special values, that will just be written in the 4 bytes range, interpreted as 32-bits>
```
(the parser should bail on values in the reserved range other than 0xffffffff, and not try to carry on reading)

or

```
Format: DWARF64 (implies 0xffffffff for first 4 bytes)
Length: <64-bit length value>
```
I guess there might be an edge case where people want DWARF32 bit 8-byte offsets or DWARF64 with 4-byte offsets, but let's not worry about that. If people actually need that behaviour, it can be added later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81063