[PATCH] DebugInfo: fix crash with null streamer

[Alp Toker]

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.

>
> 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