[PATCH] DebugInfo: fix crash with null streamer

[David Blaikie]

On Thu, Jun 19, 2014 at 10:23 AM, Alp Toker <alp at nuanti.com> wrote:
>
> On 19/06/2014 17:20, David Blaikie wrote:
>>
>> I'm still not entirely sure of the point of the null streamer (&
>> rather dislike tests that amount to "make sure this doesn't crash"),
>> but so long as we have it it seems good that it not crash.
>
>
> The use case for this has been to collect backend and ASM parser diagnostics
> in the frontend when we're not otherwise interested in generated output, but
> still want MC to run and debug locations to get processed.
>
> I originally developed raw_counting_ostream for this purpose but found that
> MCNullStreamer was able to satisfy the need with less overhead, and resorted
> to fixing it in the proposed patch.
>
> Fortunately raw_counting_ostream has since found a home tucked away as part
> of the LOH optimization (r210747). It's a neat little class -- think we
> should make it generally available from llvm/Support/raw_ostream.h?
>
> Anyway, after investigating both approaches, I was left with the impression
> thatMCNullStreamer * does* provide some validation of the streamer model so
> seems worth keeping -- though I agree "doesn't crash" tests aren't great.
>
> Architectural thoughts: It seems issues like this would go away if
> MCStreamer were to be layered as a strictly write-only interface. Right now
> there are a couple of read-backs like FileIDs and those are were potential
> inconsistencies have showed up.

While I'd really love this, even just this one place (DWARF FileIDs)
that I've looked at (I don't know what other read/write APIs
MCStreamer has) is really not obvious how to fix in my experience so
far.

With a sufficient change in abstractions it might be possible, though.

Here's the issue (vaguely) as I dabbled with this recently: DWARF line
table construction is built into the MCStreamer - my understanding is
that it needs to be at that layer since that's where relaxation
occurs, so the streamer's the only thing that knows literally how many
bytes the instruction sequence is (or, in the case of asm output, #
line directives have to be interspersed with the instruction stream).
I suppose that's the critical thing to your proposal here: to create
the line table, information from the MCStreamer is required.

This would be fine for your proposal, but for the wrinkle that the
rest of DWARF output references the line table too - the files listed
in the line table are also referenced from debug info to describe the
file location of the various debug info entities (classes, functions,
variables, etc). So we need to be able to query for existing file
numberings, and/or insert new ones (which might later be reused by the
line table data too).

That's my rough understanding, anyway.

- David

>
>
>>
>> Your change looks good to me - please commit.
>
>
> Thanks, landed in r211282!
>
> Alp.
>
>
>
>
>>
>> - David
>>
>> On Wed, Jun 18, 2014 at 11:27 PM, Alp Toker <alp at nuanti.com> wrote:
>>>
>>> On 16/06/2014 20:53, David Blaikie wrote:
>>>>
>>>> Perhaps the MCNullStreamer should provide a better stub for this
>>>> rather than returning zero?
>>>>
>>>> Given that the null streamer produces no output (I guess that's what
>>>> it's for - I haven't used it personally) I don't suppose there's much
>>>> that can be feature tested other than "this does not crash".
>>>>
>>>> It looks like you can invoke the null streamer in llc using
>>>> "-filetype=null" (I found this with some spelunking, following clang's
>>>> implementation of -emit-codegen-only down to the target layer
>>>> (CGFT_Null), then looking for that in LLVM utilities, etc...
>>>> eventually finding the cl::opt for "filetype" in
>>>> include/llvm/CodeGen/CommandFlags.h)
>>>
>>>
>>> Thanks for the spelunking David, that was enough to set things in the
>>> right
>>> direction.
>>>
>>> Following your suggestion I did a local s/filetype=\w+/filetype=null/
>>> throughout all the tests and examined the output for anything unexpected.
>>> The results were interesting..
>>>
>>> It turns out that other bits of machinery like the ASM parser also depend
>>> on
>>> valid file IDs getting generated, not just DwarfUnit.
>>>
>>> Removing two no-op overrides in MCNullStreamer kicked the underlying ID
>>> tracker back into action and fixed everything including the original
>>> issue,
>>> without having to weaken assertions as the original patch did.
>>>
>>> New fix attached complete with tests.
>>>
>>> Alp.
>>>
>>>
>>>
>>>> On Mon, Jun 16, 2014 at 10:13 AM, Alp Toker <alp at nuanti.com> wrote:
>>>>>
>>>>> The attached patch fixes crashes when emitting debug info with a
>>>>> MCNullStreamer.
>>>>>
>>>>> The only test case I could come up with uses clang's EmitBackendOutput
>>>>> in
>>>>> -emit-codegen-only mode to reproduce the crash:
>>>>>
>>>>>     echo 'void f() {}' | clang -cc1 - -gline-tables-only
>>>>> -emit-codegen-only
>>>>>
>>>>> I'm not familiar enough with backend debug testing info to pull
>>>>> together
>>>>> a
>>>>> standalone test. Suggestions welcome!
>>>>>
>>>>> Alp.
>>>>>
>>>>> --
>>>>> http://www.nuanti.com
>>>>> the browser experts
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> llvm-commits mailing list
>>>>> llvm-commits at cs.uiuc.edu
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>>
>>> --
>>> http://www.nuanti.com
>>> the browser experts
>>>
>
> --
> http://www.nuanti.com
> the browser experts
>