[llvm] r256258 - [Support] Allow multiple paired calls to {start, stop}Timer()

[Davide Italiano via llvm-commits]

This looks sensible. Can you please put it on phab so that I can
review it easily?

Thanks,

--
Davide

On Fri, May 27, 2016 at 9:24 AM, Vedant Kumar <vsk at apple.com> wrote:
> Hi Davide,
>
>> On May 26, 2016, at 7:48 PM, Davide Italiano <davide at freebsd.org> wrote:
>>
>> On Thu, May 26, 2016 at 7:41 PM, Davide Italiano <davide at freebsd.org> wrote:
>>> On Tue, Dec 22, 2015 at 9:36 AM, Vedant Kumar via llvm-commits
>>> <llvm-commits at lists.llvm.org> wrote:
>>>> Author: vedantk
>>>> Date: Tue Dec 22 11:36:17 2015
>>>> New Revision: 256258
>>>>
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=256258&view=rev
>>>> Log:
>>>> [Support] Allow multiple paired calls to {start,stop}Timer()
>>>>
>>>> Differential Revision: http://reviews.llvm.org/D15619
>>>>
>>>> Reviewed-by: rafael
>>>>
>>>
>>> I just noticed that there are cases in clang where we hit this
>>> assertion (if timers are enabled in the frontend).
>>>
>>> clang-3.9: /home/davide/work/checkout/llvm/lib/Support/Timer.cpp:139:
>>> void llvm::Timer::startTimer(): Assertion `!Running && "Cannot start a
>>> running timer"' failed.
>>>
>>> I don't have a synthetic test case (at least not one I can share
>>> easily) but this is what the stack trace looks like:
>>> #9 0x00000000022b4252
>>> clang::BackendConsumer::HandleTopLevelDecl(clang::DeclGroupRef)
>>> /home/davide/work/checkout/clang/lib/CodeGen/CodeGenAction.cpp:116:0
>>> [...]
>>> #61 0x00000000022b952c (anonymous
>>> namespace)::CodeGeneratorImpl::HandleTopLevelDecl(clang::DeclGroupRef)
>>> /home/davide/work/checkout/clang/lib/CodeGen/ModuleBuilder.cpp:147:0
>>> #62 0x00000000022b41eb
>>> clang::BackendConsumer::HandleTopLevelDecl(clang::DeclGroupRef)
>>> /home/davide/work/checkout/clang/lib/CodeGen/CodeGenAction.cpp:120:0
>>>
>>> We recurse on HandleTopLevelDecl() and inside HandleTopLevelDecl()
>>> startTimer() is called, so, the second time we call the function the timer was
>>> started already (but never stopped).
>>> I don't think this should be handled by the clients of Timers but by
>>> Timer itself.
>
> The old Timer implementation intentionally allowed this behavior.
>
> I got rid of it in r256258 because I didn't know anyone relied on it.
>
>
>>> My proposal is that of refcounting Timer so that every time we
>>> increment the refcount in situation like this and only stop the
>>> counter when the refcount goes to zero.
>
> I've attached a patch for this change + a regression test. Could you take a look?
>
>
>
>>> Thought(s) and or other opinion(s)?
>>>
>>> Thanks!
>>>
>>> --
>>> Davide
>>
>> Also, if we want to preserve the old semantic (i.e. it's not possible
>> to start an already started timer) we can provide a flag (`bool
>> canBeNested`) that controls the behavior and restore the old behavior
>> for all the timers but these ones which cause trouble.
>
> We can add this in a different patch if someone needs it.
>
> thanks,
> vedant
>
>>
>> --
>> Davide
>>
>> "There are no solved problems; there are only problems that are more
>> or less solved" -- Henri Poincare
>
>



-- 
Davide

"There are no solved problems; there are only problems that are more
or less solved" -- Henri Poincare