[Lldb-commits] [PATCH] D84008: [DWARFYAML][WIP] Refactor emitDebugInfo() to make the length be inferred.

[Xing GUO via Phabricator via lldb-commits]

Higuoxing updated this revision to Diff 279756.
Higuoxing added a comment.
Herald added subscribers: sstefan1, ormris, emaste.
Herald added a reviewer: espindola.
Herald added a reviewer: jdoerfert.

Address comments & Rebase & Add one test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84008

Files:
  lldb/unittests/TestingSupport/Symbol/YAMLModuleTester.cpp
  llvm/include/llvm/ObjectYAML/DWARFEmitter.h
  llvm/include/llvm/ObjectYAML/DWARFYAML.h
  llvm/lib/ObjectYAML/CMakeLists.txt
  llvm/lib/ObjectYAML/DWARFEmitter.cpp
  llvm/lib/ObjectYAML/DWARFVisitor.cpp
  llvm/lib/ObjectYAML/DWARFVisitor.h
  llvm/lib/ObjectYAML/DWARFYAML.cpp
  llvm/test/tools/yaml2obj/ELF/DWARF/debug-info.yaml
  llvm/unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp
  llvm/unittests/DebugInfo/DWARF/DWARFDieTest.cpp

Index: llvm/unittests/DebugInfo/DWARF/DWARFDieTest.cpp
===
--- llvm/unittests/DebugInfo/DWARF/DWARFDieTest.cpp
+++ llvm/unittests/DebugInfo/DWARF/DWARFDieTest.cpp
@@ -48,7 +48,7 @@
   - Value:   25
   )";
   Expected>> Sections =
-  DWARFYAML::emitDebugSections(StringRef(yamldata), /*ApplyFixups=*/true,
+  DWARFYAML::emitDebugSections(StringRef(yamldata),
/*IsLittleEndian=*/true);
   ASSERT_THAT_EXPECTED(Sections, Succeeded());
   std::vector Loclists{
Index: llvm/unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp
===
--- llvm/unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp
+++ llvm/unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp
@@ -1377,7 +1377,7 @@
  "  - AbbrCode:0x\n"
  "Values:\n";
 
-  auto ErrOrSections = DWARFYAML::emitDebugSections(StringRef(yamldata), true);
+  auto ErrOrSections = DWARFYAML::emitDebugSections(StringRef(yamldata));
   ASSERT_TRUE((bool)ErrOrSections);
   std::unique_ptr DwarfContext =
   DWARFContext::create(*ErrOrSections, 8);
Index: llvm/test/tools/yaml2obj/ELF/DWARF/debug-info.yaml
===
--- llvm/test/tools/yaml2obj/ELF/DWARF/debug-info.yaml
+++ llvm/test/tools/yaml2obj/ELF/DWARF/debug-info.yaml
@@ -735,3 +735,107 @@
   AbbrOffset: 0x1234
   AddrSize:   8
   Entries:[]
+
+## k) Test that yaml2obj is able to emit a correct length for compilation units.
+
+# RUN: yaml2obj --docnum=13 %s -o %t13.o
+# RUN: llvm-readelf --hex-dump=.debug_info %t13.o | \
+# RUN:   FileCheck %s --check-prefix=LENGTH
+
+#  INFER-LENGTH: Hex dump of section '.debug_info':
+# INFER-LENGTH-NEXT: 0x 3700 0400 0801  7...
+##  ^---unit_length (0x37)
+##   ^---   4-byte (accumulated length 0x04)
+##^---  4-byte (accumulated length 0x08)
+## ^--- 4-byte (accumulated length 0x0c)
+# INFER-LENGTH-NEXT: 0x0010 0c001600  1e00 2011 .. .
+##  ^---4-byte (accumulated length 0x10)
+##   ^---   4-byte (accumulated length 0x14)
+##^---  4-byte (accumulated length 0x18)
+## ^--- 4-byte (accumulated length 0x1c)
+# INFER-LENGTH-NEXT: 0x0020  3300 0220 1100 ..3 
+##  ^---4-byte (accumulated length 0x20)
+##   ^---   4-byte (accumulated length 0x24)
+##^---  4-byte (accumulated length 0x28)
+## ^--- 4-byte (accumulated length 0x2c)
+# INFER-LENGTH-NEXT: 0x0030 0006 0038 00...8...
+##  ^---4-byte (accumulated length 0x30)
+##   ^---   4-byte (accumulated length 0x34)
+##^-3-byte (accumulated length 0x37)
+
+## The handwritten DIEs should look like:
+
+## 0x000b: DW_TAG_compile_unit [1] *
+##   DW_AT_producer [DW_FORM_strp]( .debug_str[0x] = "clang version 10.0.0 ")
+##   DW_AT_language [DW_FORM_data2]   (DW_LANG_C99)
+##   DW_AT_name [DW_FORM_strp]( .debug_str[0x0016] = "hello.c")
+##   DW_AT_stmt_list 

[Lldb-commits] [PATCH] D84008: [DWARFYAML][WIP] Refactor emitDebugInfo() to make the length be inferred.

[James Henderson via Phabricator via lldb-commits]

jhenderson added a comment.

In principle, this approach looks fine, but please do as @labath suggests to 
reduce the amount of changes in one go.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84008



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D84008: [DWARFYAML][WIP] Refactor emitDebugInfo() to make the length be inferred.

[Xing GUO via Phabricator via lldb-commits]

Higuoxing added a comment.

In D84008#2161461 , @labath wrote:

> In D84008#2161243 , @Higuoxing wrote:
>
> > In D84008#2160426 , @MaskRay wrote:
> >
> > > The number of changed tests is large. Is it worth moving the 
> > > `IO.mapOptional("Length", Unit.Length);` change to a separate patch to 
> > > make the refactoring more focused? Thanks
> >
> >
> > This patch is intended to make the length field be inferred when emitting 
> > the .debug_info section. If we move the `IO.mapOption("Length", 
> > Unit.Length);` change to a separate change, we might not be able to know 
> > when to infer the length? There are two visitors, `DumpVisitor` which is 
> > used to emit the .debug_info section and `DIEFixupVisitor` which is used to 
> > calculate the length field for us. Do you mean that we keep the 
> > `DIEFixupVisitor` class and remove the `DumpVisitor` class in this patch?
>
>
> I think that should work if you make it so that this other patch comes before 
> the functional change in this patch. That other patch could change the 
> encoding to hex (`uint64_t Length;` -> `yaml::Hex64 Length;`) and make it 
> default to zero (`IO.mapRequired("Length", Unit.Length);` -> 
> `IO.mapOptional("Length", Unit.Length, 0);`). That should have no functional 
> change (I think), but allow you to make the changes in all the yaml files. 
> The visitation stuff could come after that.


Thank you @labath, It sounds good to me!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84008



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D84008: [DWARFYAML][WIP] Refactor emitDebugInfo() to make the length be inferred.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D84008#2161243 , @Higuoxing wrote:

> In D84008#2160426 , @MaskRay wrote:
>
> > The number of changed tests is large. Is it worth moving the 
> > `IO.mapOptional("Length", Unit.Length);` change to a separate patch to make 
> > the refactoring more focused? Thanks
>
>
> This patch is intended to make the length field be inferred when emitting the 
> .debug_info section. If we move the `IO.mapOption("Length", Unit.Length);` 
> change to a separate change, we might not be able to know when to infer the 
> length? There are two visitors, `DumpVisitor` which is used to emit the 
> .debug_info section and `DIEFixupVisitor` which is used to calculate the 
> length field for us. Do you mean that we keep the `DIEFixupVisitor` class and 
> remove the `DumpVisitor` class in this patch?


I think that should work if you make it so that this other patch comes before 
the functional change in this patch. That other patch could change the encoding 
to hex (`uint64_t Length;` -> `yaml::Hex64 Length;`) and make it default to 
zero (`IO.mapRequired("Length", Unit.Length);` -> `IO.mapOptional("Length", 
Unit.Length, 0);`). That should have no functional change (I think), but allow 
you to make the changes in all the yaml files. The visitation stuff could come 
after that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84008



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D84008: [DWARFYAML][WIP] Refactor emitDebugInfo() to make the length be inferred.

[Xing GUO via Phabricator via lldb-commits]

Higuoxing added a comment.

In D84008#2160426 , @MaskRay wrote:

> The number of changed tests is large. Is it worth moving the 
> `IO.mapOptional("Length", Unit.Length);` change to a separate patch to make 
> the refactoring more focused? Thanks


This patch is intended to make the length field be inferred when emitting the 
.debug_info section. If we move the `IO.mapOption("Length", Unit.Length);` 
change to a separate change, we might not be able to know when to infer the 
length? There are two visitors, `DumpVisitor` which is used to emit the 
.debug_info section and `DIEFixupVisitor` which is used to calculate the length 
field for us. Do you mean that we keep the `DIEFixupVisitor` class and remove 
the `DumpVisitor` class in this patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84008



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D84008: [DWARFYAML][WIP] Refactor emitDebugInfo() to make the length be inferred.

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added a comment.

The number of changed tests is large. Is it worth moving the 
`IO.mapOptional("Length", Unit.Length);` change to a separate patch to make the 
refactoring more focused? Thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84008



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits