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

[Justin Bogner]

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?

-------------- next part --------------
A non-text attachment was scrubbed...
Name: exitblock.patch
Type: text/x-patch
Size: 1828 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150501/9c6f8c93/attachment.bin>
-------------- next part --------------

> 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