[Lldb-commits] [PATCH] D40211: Add comments to DWARFCompileUnit length fields/methods

[Robinson, Paul via lldb-commits]

FTR, the size of the compile-unit header also changed in DWARF version 5, independent of the 32/64 format.

On a different topic, I had thought there was a goal of nuking lldb's copy of the DWARFxxx headers and converting to use LLVM's?  Did I imagine this?  If I do remember that correctly, fiddling with lldb's copy doesn't make much sense.
--paulr

From: lldb-commits [mailto:lldb-commits-bounces at lists.llvm.org] On Behalf Of Zachary Turner via lldb-commits
Sent: Monday, November 20, 2017 8:17 AM
To: Greg Clayton
Cc: Jan Kratochvil via Phabricator; lldb-commits at lists.llvm.org; reviews+D40211+public+c1500ec8aeff1f84 at reviews.llvm.org; jdevlieghere at apple.com; jan.kratochvil at redhat.com
Subject: Re: [Lldb-commits] [PATCH] D40211: Add comments to DWARFCompileUnit length fields/methods

You can make structs that are host and byte-order independent, LLVM is filled with stuff like this.  And while you might end up processing the information off in a way that it can be stored in a single compile unit without such a struct, it still can be useful when you're actually *doing* the parsing.

For one, it serves as documentation of the format.  It sounds like in order to know what a DWARF64 header looks like currently, I have to go read the code that actually parses one, which might require me to understand several functions and follow some pointer arithmetic and various other stuff.

Second, it can converts code like this:
```
if (m_dwarf64) {
  // read the first field
  // adjust a pointer
  // read the second field
  // adjust a pointer
  // read the third field
  // adjust a pointer
  ...

  // store the fields in the CompileUnit
} else {
  // read the first field
  // adjust a pointer
  // read the second field
  // adjust a pointer
  // read the third field
  // adjust a pointer
  ...
  // store the fields in the CompileUnit
}
```
into this:
```
if (m_dwarf64) {
  DWARF64_HEADER h;
  readStructure(h);
  // store the fields in the CompileUnit
} else {
  DWARF32_HEADER h;
  readStructure(h);
  // store the fields in the CompileUnit
}
```

Anyway, you answered my question, which is that we don't have one

On Mon, Nov 20, 2017 at 8:07 AM Greg Clayton <clayborg at gmail.com<mailto:clayborg at gmail.com>> wrote:
sizeof(struct) tends to include system level padding for the current host. But to answer your question, no there isn't a structure defined like this and we wouldn't use them anyway as we want to fill out one compile unit struct that works for both.

On Nov 20, 2017, at 8:01 AM, Zachary Turner <zturner at google.com<mailto:zturner at google.com>> wrote:

Right but isn’t there a DWARF64_HEADER and DEARF32_HEADER struct somewhere? This way you could just say

return m_isdwarf64 ? sizeof(DWARF64_HEADER) : sizeof(DWARF32_HEADER);
On Mon, Nov 20, 2017 at 7:50 AM Greg Clayton <clayborg at gmail.com<mailto:clayborg at gmail.com>> wrote:

On Nov 19, 2017, at 4:56 PM, Zachary Turner <zturner at google.com<mailto:zturner at google.com>> wrote:


On Sun, Nov 19, 2017 at 6:35 AM Jan Kratochvil via Phabricator via lldb-commits <lldb-commits at lists.llvm.org<mailto:lldb-commits at lists.llvm.org>> wrote:
This revision was automatically updated to reflect the committed changes.
Closed by commit rL318626: Add comments to DWARFCompileUnit length fields/methods (authored by jankratochvil).

Changed prior to commit:
  https://reviews.llvm.org/D40211?vs=123472&id=123498#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D40211

Files:
  lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.h


Index: lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.h
===================================================================
--- lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.h
+++ lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.h
@@ -41,26 +41,24 @@
   void Clear();
   bool Verify(lldb_private::Stream *s) const;
   void Dump(lldb_private::Stream *s) const;
+  // Offset of the initial length field.
   dw_offset_t GetOffset() const { return m_offset; }
   lldb::user_id_t GetID() const;
-  uint32_t Size() const {
-    return m_is_dwarf64 ? 23
-                        : 11; /* Size in bytes of the compile unit header */
-  }
+  // Size in bytes of the initial length + compile unit header.
+  uint32_t Size() const { return m_is_dwarf64 ? 23 : 11; }

This is pretty gross.  Don't we have a structure somewhere that represents a compile unit header?  That we can just call sizeof on?  Same goes for the rest of the patch

It varies depending on data on how the length unit is represented in the data stream. If the length starts with UINT32_MAX, it is followed by a 64 bit length. If the length isn't UINT32_MAX it is just a 32 bit length.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20171120/511d9ab0/attachment-0001.html>