[PATCH] D66411: Fix -ftime-trace breaking flame-graph assumptions

Tue Aug 20 12:02:51 PDT 2019

rnk added inline comments.

================
Comment at: llvm/unittests/Support/TimeProfilerTest.cpp:19
+// Run enough iterations to reliably see this issue in Release.
+#define ITERATIONS 100000
+#else
----------------
russell.gallop wrote:
> rnk wrote:
> > I'm not sure this test has that much value because it will not fail deterministically if you revert the fix to bring back the truncation bugs.
> Thanks for the review. I agree it's not ideal, this was the best way I could think of for testing this behaviour.
> 
> It might be possible to set the time values to appropriate values to trigger this but that would require exposing a fair bit more of TimeProfiler.cpp.
> 
> One alternative might be to have a check in Write() that reconstructed the flame graph and asserted if there was a problem. It would require a very large file (or a lot of files) to reliably hit the issue though.
> 
> Or is this okay to send without a test?
Test aside, adding the assert seems valuable. As it is written now, it should hold unless the steady clock lies, right?

For the test, how long does the test normally take? If it takes more than 8s-10s, I think I would prefer to remove it and commit this without a test.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66411/new/

https://reviews.llvm.org/D66411