3.6.1 Nomination: r232438, r232443, and clang/r232439, which fix pr22436
Justin Bogner
mail at justinbogner.com
Fri May 1 21:30:02 PDT 2015
Tom Stellard <tom at stellard.net> writes:
> 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.
SGTM
> -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
More information about the llvm-commits
mailing list