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

[Vedant Kumar via llvm-commits]

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?

-------------- next part --------------
A non-text attachment was scrubbed...
Name: Timer-nesting.diff
Type: application/octet-stream
Size: 17835 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160527/fef0533b/attachment.obj>
-------------- next part --------------

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