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

Tue Aug 20 07:52:40 PDT 2019

russell.gallop 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
----------------
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?

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

https://reviews.llvm.org/D66411