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

[Tom Stellard]

On Fri, May 01, 2015 at 10:15:33AM -0700, 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.
> 

Since -rc1 is coming on Monday, I'm going to assume that we are going
with solution number 2 unless I hear otherwise.

-Tom

> Nick, WDYT?
> 

> diff --git a/include/llvm/Transforms/Instrumentation.h b/include/llvm/Transforms/Instrumentation.h
> index 0de2d75..24e3ef7 100644
> --- a/include/llvm/Transforms/Instrumentation.h
> +++ b/include/llvm/Transforms/Instrumentation.h
> @@ -59,10 +59,6 @@ struct GCOVOptions {
>    // Emit the name of the function in the .gcda files. This is redundant, as
>    // the function identifier can be used to find the name from the .gcno file.
>    bool FunctionNamesInData;
> -
> -  // Emit the exit block immediately after the start block, rather than after
> -  // all of the function body's blocks.
> -  bool ExitBlockBeforeBody;
>  };
>  ModulePass *createGCOVProfilerPass(const GCOVOptions &Options =
>                                     GCOVOptions::getDefault());
> diff --git a/lib/Transforms/Instrumentation/GCOVProfiling.cpp b/lib/Transforms/Instrumentation/GCOVProfiling.cpp
> index ec8e86b..60b541f 100644
> --- a/lib/Transforms/Instrumentation/GCOVProfiling.cpp
> +++ b/lib/Transforms/Instrumentation/GCOVProfiling.cpp
> @@ -57,7 +57,6 @@ GCOVOptions GCOVOptions::getDefault() {
>    Options.UseCfgChecksum = false;
>    Options.NoRedZone = false;
>    Options.FunctionNamesInData = true;
> -  Options.ExitBlockBeforeBody = DefaultExitBlockBeforeBody;
>  
>    if (DefaultGCOVVersion.size() != 4) {
>      llvm::report_fatal_error(std::string("Invalid -default-gcov-version: ") +
> @@ -519,7 +518,7 @@ void GCOVProfiler::emitProfileNotes() {
>  
>        Funcs.push_back(make_unique<GCOVFunction>(SP, &out, FunctionIdent++,
>                                                  Options.UseCfgChecksum,
> -                                                Options.ExitBlockBeforeBody));
> +                                                DefaultExitBlockBeforeBody));
>        GCOVFunction &Func = *Funcs.back();
>  
>        for (Function::iterator BB = F->begin(), E = F->end(); BB != E; ++BB) {

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