3.6.1 Nomination: r232438, r232443, and clang/r232439, which fix pr22436

Nick Lewycky nicholas at mxc.ca
Sun May 3 18:20:33 PDT 2015


Justin Bogner wrote:
> Tom Stellard<tom at stellard.net>  writes:
>> On Thu, Apr 30, 2015 at 11:23:31PM -0400, Tom Stellard wrote:
>>> Hi Justin,
>>>
>>> I've merged these patches:
>>>
>>> On Wed, Apr 29, 2015 at 10:00:15AM -0700, Justin Bogner wrote:
>>>> Hey Tom, can you cherry pick these three patches, which collectively fix
>>>> llvm.org/pr22436, to the 3.6 branch? I'm the code owner for profiling
>>>> and coverage instrumentation and this is a pretty straightforward bug
>>>> fix.
>>>>
>>>>    llvm/r232438:
>>>
>>> r236303
>>>
>>
>> Hi Justin,
>>
>> It turns out that this commit breaks the libLLVM.so ABI, because the
>> size of the GCOVOptions class.  I think we may have to revert this
>> (and the corresponding clang patch) unless we can come up with and
>> alternative solution.  Do you have any ideas?
>
> The bug is that clang generates gcov data that llvm-cov can't read, due
> to the change in r223193, which was before 3.6 branched. The fix that
> this patch series implements is making the new behaviour in r223193
> optional and off-by-default. There are a couple of approaches we could
> take without changing the GCOVOptions class:
>
> 1. Hardcode the option off, effectively reverting to pre-r223193
>     behaviour.
>
> 2. Remove the clang option so we don't need to pass information via
>     GCOVOptions, but leave the llvm-level option. This would essentially
>     keep the optional behaviour available but make it harder to turn on.
>
> 3. Pass the option through some auxiliary structure so that we don't
>     muck with GCOVOptions, but can still have a way to toggle this from
>     clang.
>
> 4. Just revert these, and live with the fact that llvm-cov and clang
>     can't be used together.
>
> I'm not a fan of (4), and I suspect that (2) is the least risky code
> change. I've attached a patch against 3.6 for (2) - we'll need to revert
> the clang change (r236305) if we go that way.
>
> Nick, WDYT?

I probably would've gone with #3 in a special patch for the branch only. 
#2 is the next best option. Agreed that #4 and #1 are really bad.

>> Here is the relevant information from the ABI compatibility report:
>>
>> --------------------------------------------------------------------------------
>>
>> class GCOVOptions:
>>
>> Size of this class has been changed from 9 bytes to 10 bytes.
>>
>> 1) The class has only inline or auto-generated constructors which will be
>> copied to applications at compile time and will allocate an older memory
>> layout. Call of any exported method of this class may access a memory
>> outside the allocated objects or inside the older memory structure and
>> result in crash or incorrect behavior of applications.
>>
>> 2) The memory layout and size of subclasses will be changed.
>>
>> --------------------------------------------------------------------------------
>>
>> Thanks,
>> Tom
>>
>>>>
>>>>      GCOV: Make the exit block placement from r223193 optional
>>>>
>>>>      By default we want our gcov emission to stay 4.2 compatible, which
>>>>      means we need to continue emit the exit block last by default. We add
>>>>      an option to emit it before the body for users that need it.
>>>>
>>>>    llvm/r232443:
>>>
>>> r236304
>>>
>>>>
>>>>      llvm-cov: Warn instead of error if a .gcda has arcs from an exit block
>>>>
>>>>      Patch by Vanderson M. Rosario. Thanks!
>>>>
>>>>    clang/r232439:
>>>
>>> r236305
>>>
>>>>
>>>>      GCOV: Expose the -coverage-exit-block-before-body flag in clang -cc1
>>>>
>>>>      This exposes the optional exit block placement logic from r232438 as a
>>>>      clang -cc1 option. There is a test on the llvm side, but there isn't
>>>>      really a way to inspect the gcov options from clang to test it here as
>>>>      well.
>>>
>>> -Tom
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits




More information about the llvm-commits mailing list