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

[Tom Stellard]

On Sun, May 03, 2015 at 06:20:33PM -0700, Nick Lewycky wrote:
> 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.
> 

I reverted the clang changes and applied the patch in r236445.  If we 
really want to do something like #3, maybe we can re-visit this for 3.6.2

-Tom

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