This is an archive of the discontinued LLVM Phabricator instance.

fix bug 15393: invalid dwarf info on Win64
ClosedPublic

Authored by vtjnash on Jun 25 2016, 8:40 PM.

Details

Summary

The relocations for DIEEntry::EmitValue were wrong for Win64 (emitting FK_Data_4 instead of FK_SecRel_4). This corrects that oversight so that the DWARF data is correct in Win64 COFF files.

https://llvm.org/bugs/show_bug.cgi?id=15393

Based on a patch by David (https://ghostbin.com/paste/ucbr6)

Diff Detail

Repository
rL LLVM

Event Timeline

vtjnash updated this revision to Diff 61899.Jun 25 2016, 8:40 PM
vtjnash retitled this revision from to fix bug 15393: invalid dwarf info on Win64.
vtjnash updated this object.
vtjnash added reviewers: rafael, majnemer.
vtjnash set the repository for this revision to rL LLVM.
vtjnash added subscribers: loladiro, tkelman.
majnemer edited edge metadata.Jun 25 2016, 8:53 PM

This needs a test.

majnemer edited edge metadata.Jun 25 2016, 8:54 PM
majnemer added a subscriber: llvm-commits.

Adding llvm-commits

vtjnash updated this revision to Diff 78816.Nov 21 2016, 6:10 PM

This updates the ref_addr_relocation test to have better coverage of the various combinations of DWARF versions and architectures. Because the debug info framework in LLVM expects that .secrel32 .debug_info + offset exists (and LLVM also tests that the debug info isn't using labels that would be supported everywhere: "test that we don't create useless labels"), this also makes that a real relocation type and handles the printing and parsing of it.

majnemer accepted this revision.Nov 21 2016, 9:58 PM
majnemer edited edge metadata.

LGTM with nits addressed.

lib/MC/MCAsmStreamer.cpp
621 ↗(On Diff #78816)

Please format this appropriately.

lib/MC/MCParser/COFFAsmParser.cpp
469–473 ↗(On Diff #78816)

Please include a check for < UINT32_MAX.

lib/MC/WinCOFFStreamer.cpp
202 ↗(On Diff #78816)

Comments end with a period.

This revision is now accepted and ready to land.Nov 21 2016, 9:58 PM
vtjnash updated this revision to Diff 79015.Nov 22 2016, 7:00 PM
vtjnash edited edge metadata.
vtjnash removed rL LLVM as the repository for this revision.

address review comments

vtjnash marked 3 inline comments as done.Nov 22 2016, 7:02 PM
This revision was automatically updated to reflect the committed changes.

I reverted this because of the following buildbot failure: http://bb.pgr.jp/builders/ninja-x64-msvc-RA-centos6/builds/32492. @vtjnash please take a look and see if you have a fix.

the warnings in that test are from a different, related bug. An approximate correction for that is:

diff --git a/lib/CodeGen/AsmPrinter/DIE.cpp b/lib/CodeGen/AsmPrinter/DIE.cpp
index a328851..73977f2 100644
--- a/lib/CodeGen/AsmPrinter/DIE.cpp
+++ b/lib/CodeGen/AsmPrinter/DIE.cpp
@@ -495,7 +495,8 @@ void DIELabel::EmitValue(const AsmPrinter *AP, dwarf::Form Form) const {
   AP->EmitLabelReference(Label, SizeOf(AP, Form),
                          Form == dwarf::DW_FORM_strp ||
                              Form == dwarf::DW_FORM_sec_offset ||
-                             Form == dwarf::DW_FORM_ref_addr);
+                             Form == dwarf::DW_FORM_ref_addr ||
+                             Form == dwarf::DW_FORM_data4);
 }
 
 /// SizeOf - Determine size of label value in bytes.

which should make the Dwarf 2 info more valid (I was only focused on Dwarf 4 information in this PR, which doesn't have the bug in the spec which makes that above diff necessary):

The data in DW_FORM_data1, DW_FORM_data2, DW_FORM_data4 and DW_FORM_data8 can be anything. Depending on context, it may be an offset to a debugging information entry, a signed integer, an unsigned integer, a floating-point constant, or anything else. A consumer must use context to know how to interpret the bits, which if they are target machine data (such as an integer or floating point constant) will be in target machine byte-order.

The test failure is because we missed a case when developing that relocation code:

diff --git a/lib/CodeGen/AsmPrinter/AsmPrinter.cpp b/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
index 9eba56d..263c9d6 100644
--- a/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
+++ b/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
@@ -1689,6 +1689,8 @@ void AsmPrinter::EmitLabelPlusOffset(const MCSymbol *Label, uint64_t Offset,
                                      bool IsSectionRelative) const {
   if (MAI->needsDwarfSectionOffsetDirective() && IsSectionRelative) {
     OutStreamer->EmitCOFFSecRel32(Label, Offset);
+    if (Size > 4)
+      OutStreamer->EmitZeros(Size - 4);
     return;
   }

I'm pretty sure this should have an assertion too (Size < UINT32_MAX && MAI->isLittleEndian()). What kind of assert should I use in this case?

We can make this test fail on all buildbots by fixing the triple. Is there any reason not to do that?

diff --git a/test/Linker/type-unique-type-array-a.ll b/test/Linker/type-unique-type-array-a.ll
index f6c70df..60817ca 100644
--- a/test/Linker/type-unique-type-array-a.ll
+++ b/test/Linker/type-unique-type-array-a.ll
@@ -1,6 +1,8 @@
-; REQUIRES: default_triple, object-emission
+; REQUIRES: object-emission
 ;
-; RUN: llvm-link %s %p/type-unique-type-array-b.ll -S -o - | %llc_dwarf -filetype=obj -O0 | llvm-dwarfdump -debug-dump=info - | FileCheck %s
+; RUN: llvm-link %s %p/type-unique-type-array-b.ll -S -o - | %llc_dwarf -mtriple=x86_64-linux-gnu -filetype=obj -O0 | llvm-dwarfdump -debug-dump=info - | FileCheck %s
+; RUN: llvm-link %s %p/type-unique-type-array-b.ll -S -o - | %llc_dwarf -mtriple=x86_64-apple-darwin10 -filetype=obj -O0 | llvm-dwarfdump -debug-dump=info - | FileCheck %s
+; RUN: llvm-link %s %p/type-unique-type-array-b.ll -S -o - | %llc_dwarf -mtriple=x86_64-pc-mingw32 -filetype=obj -O0 | llvm-dwarfdump -debug-dump=info - | FileCheck %s
 ;
 ; rdar://problem/17628609
 ;