[PATCH] Debug Info on Windows

[David Blaikie]

On Fri, Jul 26, 2013 at 10:44 AM, Carlo Kok <ck at remobjects.com> wrote:
> Op 26-7-2013 19:26, David Blaikie schreef:
>
>>>>
>>>> Can you provide test coverage for this?
>
>
>> If possible, we're trying to make a habit of including the original
>> (C, in your case I assume) source & Clang command line (ideally
>> produced from Clang since it's generally accessible to LLVM developers
>> - if it's something that can only be produced by some other frontend,
>> I'd be curious to hear about that too) in the LLVM debug info test
>> cases if possible. (& keep the IR an exact copy of what Clang
>> produced, rather than anything hand-reduced unless there's a good
>> reason to do so)
>
>
> Oke. I presumed that shorter was better, but sure. Fixed testcase attached.

Generally shorter is better - I appreciate the effort. Still
considering/figuring out how best to manage such test cases owing to
how awkward it is to maintain (updating test cases when the schema
changes, etc) the textual metadata representation of debug info.

>> (oh, and your patch uses naming inconsistent with LLVM's naming
>> convention (value should be Value))
>
>
> Also updated to use V as it uses in the case before it.

Sounds good

> The only thing I'm
> worried about is DIEValue::isLabel, last time I checked in some code with an
> enum defined as part of a class I did it wrong (VC is a lot more relaxed in
> the enum rules).

Hmm, not quite sure what the concern is there, but I do have a couple
of other questions:

a) Eric recently checked in some changes that add a DIEString type -
does this have any effect on your code?
b) Could we sink this logic down into DIELabel (or
DIEString)::EmitValue rather than having the special case up here in
emitDIE?

It'd still need to query the needsDwarfSectionOffsetDirective, I guess
(I haven't sat down to figure out what that function actually means or
how it is the right check for your situation, though, I must admit)
but not the "isLabel" case (nor the DIELabel cast)

If the code does stay where it is, the right check for the first
condition of that' if' is probably dyn_cast<DIELabel>(V) (& saving the
result of that cast so you don't have to cast again inside the
conditional)

>
> --
> Carlo Kok