r267904 - Debug info: Apply an artificial debug location to __cyg_profile_func.* calls.

[David Blaikie via cfe-commits]

On Thu, Apr 28, 2016 at 4:42 PM, Adrian Prantl <aprantl at apple.com> wrote:

>
> On Apr 28, 2016, at 4:31 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Thu, Apr 28, 2016 at 1:11 PM, Adrian Prantl <aprantl at apple.com> wrote:
>
>>
>> On Apr 28, 2016, at 12:53 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>>
>>
>> On Thu, Apr 28, 2016 at 12:50 PM, Adrian Prantl <aprantl at apple.com>
>> wrote:
>>
>>>
>>> On Apr 28, 2016, at 12:34 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>>
>>> Should these have no/artificial location? It seems like perhaps they
>>> should have the same location as the scope they're for? (well, the
>>> beginning or end of that scope, respectively, etc)
>>>
>>>
>>> While there is usually a single source location for the beginning of the
>>> function, there can be more than one such location for the end of the
>>> function, so I don’t think this is generally possible.
>>>
>>
>> Seems to me like it would be - declare a variable with a non-trivial dtor
>> at the start of the function. Whichever line the dtor call is attributed
>> to, that's the end of the function. (I think we use the } for this, or
>> perhaps the final return statement if there is one and only one - I forget
>> the specifics)
>>
>>
>> Ah yes, I guess we could be using the cleanup location of the function
>> for this.
>>
>>
>>
>>> The artificial location unambiguously marks the function call as
>>> something that does not appear in source code. For a (terrible) example
>>> (given the nature of these function calls :-p) if you look at a profile you
>>> wouldn’t want these function calls to be misattributed to any line in the
>>> source code.
>>>
>>
>> I'm not quite sure what the ramifications of that would be/why that would
>> be bad (or good) - could you explain further?
>>
>>
>> If the compiler generates code that does not have any meaningful source
>> location (in this case one could argue that ‘{‘ and ‘}’ might be meaningful
>> source locations?) then I wouldn’t want that code to be attributed to a
>> source location just because the instructions immediately follow some
>> completely unrelated instructions that happen to have a source location.
>>
>
> Sorry, I meant specifically what would be the impact of giving it the
> non-artificial location (the same as the location of the cleanup code, as
> you noted), not the impact of not giving it a location at all.
>
>
>> - Profiling: It shouldn't show up in a profile counted towards the source
>> location of the instructions before it.
>> - Debugging: Since there is no source code for it a source-based debugger
>> (you can still switch to disassembly) should treat the function call as
>> transparent and skip over it as if it didn’t exist.
>>
>
> Much like the non-trivial dtor, just because the user didn't explicitly
> write the function call doesn't seem to me to mean it should be invisible.
>
>
> The user may not have written the call to the dtor, but the user did at
> least write the dtor itself; and there exists source code for it.
>

The user may not have written the dtor - it could be non-trivial but
implicit (eg: write a struct that contains a std::string member (yes, the
std::string's dtor is user-defined, but the struct's dtor is not)).


>
> The profiling argument sort of makes sense to me - but it's going to be
> attributed to /something/, right? (& wait - aren't these functions the
> profiling mechanisms themselves for counter based (rather than sample
> based) profiling?
>
>
> Yes they are, that’s why I said at the beginning that this example was a
> terrible one :-)
>
> Do people sample /and/ counter profile the same binary?)
>
>
> Hopefully not. This was really just meant as an example why someone would
> choose an artificial location over a concrete one in similar situations. In
> the end, I’m not particularly attached to using the artificial location
> here, but these were the reasons that made me opt for an artificial
> location here.
>
> -- adrian
>
>
> - Dave
>
>
>>
>> -- adrian
>>
>>
>> - Dave
>>
>>
>>>
>>> -- adrian
>>>
>>>
>>> On Thu, Apr 28, 2016 at 10:21 AM, Adrian Prantl via cfe-commits <
>>> cfe-commits at lists.llvm.org> wrote:
>>>
>>>> Author: adrian
>>>> Date: Thu Apr 28 12:21:56 2016
>>>> New Revision: 267904
>>>>
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=267904&view=rev
>>>> Log:
>>>> Debug info: Apply an artificial debug location to __cyg_profile_func.*
>>>> calls.
>>>> The LLVM Verifier expects all inlinable calls in debuggable functions to
>>>> have a location.
>>>>
>>>> rdar://problem/25818489
>>>>
>>>> Modified:
>>>>     cfe/trunk/lib/CodeGen/CodeGenFunction.cpp
>>>>     cfe/trunk/test/CodeGen/instrument-functions.c
>>>>
>>>> Modified: cfe/trunk/lib/CodeGen/CodeGenFunction.cpp
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenFunction.cpp?rev=267904&r1=267903&r2=267904&view=diff
>>>>
>>>> ==============================================================================
>>>> --- cfe/trunk/lib/CodeGen/CodeGenFunction.cpp (original)
>>>> +++ cfe/trunk/lib/CodeGen/CodeGenFunction.cpp Thu Apr 28 12:21:56 2016
>>>> @@ -401,6 +401,7 @@ bool CodeGenFunction::ShouldInstrumentFu
>>>>  /// instrumentation function with the current function and the call
>>>> site, if
>>>>  /// function instrumentation is enabled.
>>>>  void CodeGenFunction::EmitFunctionInstrumentation(const char *Fn) {
>>>> +  auto NL = ApplyDebugLocation::CreateArtificial(*this);
>>>>    // void __cyg_profile_func_{enter,exit} (void *this_fn, void
>>>> *call_site);
>>>>    llvm::PointerType *PointerTy = Int8PtrTy;
>>>>    llvm::Type *ProfileFuncArgs[] = { PointerTy, PointerTy };
>>>>
>>>> Modified: cfe/trunk/test/CodeGen/instrument-functions.c
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/instrument-functions.c?rev=267904&r1=267903&r2=267904&view=diff
>>>>
>>>> ==============================================================================
>>>> --- cfe/trunk/test/CodeGen/instrument-functions.c (original)
>>>> +++ cfe/trunk/test/CodeGen/instrument-functions.c Thu Apr 28 12:21:56
>>>> 2016
>>>> @@ -1,9 +1,9 @@
>>>> -// RUN: %clang_cc1 -S -emit-llvm -o - %s -finstrument-functions |
>>>> FileCheck %s
>>>> +// RUN: %clang_cc1 -S -debug-info-kind=standalone -emit-llvm -o - %s
>>>> -finstrument-functions | FileCheck %s
>>>>
>>>>  // CHECK: @test1
>>>>  int test1(int x) {
>>>> -// CHECK: __cyg_profile_func_enter
>>>> -// CHECK: __cyg_profile_func_exit
>>>> +// CHECK: call void @__cyg_profile_func_enter({{.*}}, !dbg
>>>> +// CHECK: call void @__cyg_profile_func_exit({{.*}}, !dbg
>>>>  // CHECK: ret
>>>>    return x;
>>>>  }
>>>>
>>>>
>>>> _______________________________________________
>>>> cfe-commits mailing list
>>>> cfe-commits at lists.llvm.org
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>
>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160429/972c5117/attachment-0001.html>