[llvm] r187656 - Bugfix for making the DWARF debug strings and labels to code emitted as secrel32 instead of long opcodes (only for coff). This makes them debuggable with GDB (with fix for 64bits msvc)

[Carlo Kok]

> If I may throw in my 2c:
>
> Carlo, I am afraid it may be not as simple as excluding those 3 DW_FORM's.
> Please check out the equivalent GCC source here:
> https://github.com/mirrors/gcc/blob/master/gcc/dwarf2out.c and search it
> for calls to dw2_asm_output_offset (which eventually expands to .secrel32).
> You'll see that the decision of whether to use secrel32 depends not only
> on DW_FORM, but also possibly on section into which information is
> emitted (the 'for_eh' flag, meaning "for exception handling" I guess).

I can check, but I'm fairly sure that the EH code doesn't use the same

> Now, it appears to me that LLVM relies on the assembler to emit stack
> unwinding tables for exception handling, because I don't see that stuff
> in .s files), so this issue may be moot...  On the other hand, what
> happens when LLVM emits binary code directly?  Does code still pass
> through AsmPrinter?   Does LLVM use DIE* classes when generating unwind
> tables?..

AsmPrinter is used for both .o and .s.

>
> At the very least, I would suggest to make the test for whether or not
> to use secrel32 (in DIELabel::EmitValue) to be "opt-in", not "opt-out".
>    According to DRAWF4 spec here <http://dwarfstd.org/doc/DWARF4.pdf>,
> page 142, only DW_FORM_ref_addr, DW_FORM_sec_offset, DW_FORM_strp and
> DW_OP_call_ref are supposed to be cross- debug section references, so I
> would check for those.  Unfortunately, following this path you may find
> out that certain DIE's currently use the wrong form of encoding, so that
> may need to be fixed as well.  For example, I've found this to be the
> case for the compile unit DIE (please see my patch attached to LLVM bug
> 16249 <http://llvm.org/bugs/show_bug.cgi?id=16249>).

I'll check tomorrow for how well the reversal of this works.

> Also, I think that "IsAddress" parameter you've added to
> EmitLabelPlusOffset(), has the wrong default - it should be "true" to
> preserve the original behavior in case it's used to emit some custom
> section other than "debug-info".  In fact, I would suggest to rename it
> to "IsSectionRelative = false", for clarity.

Sounds good.

>
> Finally, I want to apologize in advance if this appears
> non-constructive.  I would like to have fixed this myself, but this area
> is complex and under-documented, especially on the subject of secrel32
> relocations (which seem to be PE-ELF specific), and I felt that there
> would be a very low chance of me getting this right without studying
> this stuff at length first.  I was hoping that an area expert, who
> alreadsy knows how this is supposed to work, would have much easier
> time.  Or at least to chime in with suggestions which way they'd like to
> see this fixed.

So far nobody showed up fitting that criteria, so we can choose to wait 
or try and improve it incrementally, I really would like to see Windows 
support working after all. I think if we combine your changes with mine 
we should be able to get pretty far already.

--
Carlo Kok