Page MenuHomePhabricator

[llvm-locstats] Report a warning if overflow was detected by llvm-dwarfdump
ClosedPublic

Authored by djtodoro on Sep 28 2021, 5:09 AM.

Details

Summary

Catch that llvm-dwarfdump detected an overflow in statistics.

Diff Detail

Event Timeline

djtodoro created this revision.Sep 28 2021, 5:09 AM
djtodoro requested review of this revision.Sep 28 2021, 5:09 AM

This change looks sensible - although it seems unusual that we don't forward any non-fatal errors from dwarfdump in locstats. Is this just the only warning that may affect the output of locstats, or are there likely to be more?

@StephenTozer Thanks for your comment.

Oh yes... we should start checking all the errors coming from llvm-dwarfdump, I'll put a TODO comment for that.

djtodoro updated this revision to Diff 377151.Oct 5 2021, 4:27 AM
  • adding todo marker for handling other errs from llvm-dwarfdump
djtodoro updated this revision to Diff 377757.Oct 7 2021, 1:21 AM
djtodoro retitled this revision from [llvm-locstats] Report a warnning if overflow was detected by llvm-dwarfdump to [llvm-locstats] Report an error if overflow was detected by llvm-dwarfdump.
djtodoro edited the summary of this revision. (Show Details)
  • parse special value for the fields that overflow
dblaikie added inline comments.Oct 7 2021, 9:44 AM
llvm/utils/llvm-locstats/llvm-locstats.py
218–220

Should it stop after the first overflowed field, or continue to print other stats?

djtodoro added inline comments.Oct 7 2021, 1:39 PM
llvm/utils/llvm-locstats/llvm-locstats.py
218–220

Some stat fields are being calculated from the other, so I guess it is better not to show any stats, than to show wrong stats.

dblaikie added inline comments.Oct 7 2021, 1:47 PM
llvm/utils/llvm-locstats/llvm-locstats.py
218–220

I'd figure not to present incorrect stats - but render those tainted stats as tainted/invalid/incomputable in some way. (worst case they could say "overflow" too, though that could be slightly misleading/not-the-whole-story)

djtodoro added inline comments.Oct 8 2021, 2:43 AM
llvm/utils/llvm-locstats/llvm-locstats.py
218–220

Hmm, it makes sense. There are some stats fields that are very important (such as bytes in parent scope), and if they overflow, we have to taint all the locstats fields.

djtodoro updated this revision to Diff 378611.Oct 11 2021, 3:26 AM
  • introduce special "tainted" value for stats that overflowed
djtodoro retitled this revision from [llvm-locstats] Report an error if overflow was detected by llvm-dwarfdump to [llvm-locstats] Report a warning if overflow was detected by llvm-dwarfdump.Oct 11 2021, 3:26 AM

I guess this is ok to go as well.

@dblaikie does this look ok now? This patch is the only one remaining from the stack.

dblaikie accepted this revision.Oct 21 2021, 10:56 AM

Eh, this code is pretty far out of my wheelhouse - don't write much Python and haven't dealt with this particular code much. I guess ideally each of the taintable variables would be explicitly tested - since each has separate code to handle it/could have separate bugs, but otherwise seems OK-ish enough to commit & other folks can chime in post-commit with further thoughts.

This revision is now accepted and ready to land.Oct 21 2021, 10:56 AM
thakis added a subscriber: thakis.Oct 27 2021, 5:59 AM

Shouldn't the test for this be in llvm/test/tools/llvm-locstats/? Seems a bit weird to have a test for a llvm-locstats behavior change in llvm/test/tools/llvm-dwarfdump.

It may make sense I think... The llvm-locstats tool depends on llvm-dwarfdump, since it is just a front-end/pretty-printer of some statistics fields. By moving it into the llvm/test/tools/llvm-locstats/, we will have duplicated text of the test, but I guess it is acceptable, right?

Also, given that we now unconditionally depend on llvm-locstats, should we remove config.llvm_locstats_used and delete llvm/test/tools/llvm-locstats/lit.local.cfg?

(Also, why is the tool in llvm/utils but the tests in llvm/test/tools?)

Also, given that we now unconditionally depend on llvm-locstats, should we remove config.llvm_locstats_used and delete llvm/test/tools/llvm-locstats/lit.local.cfg?

Oh, got it... it should actually go into its own directory...

(Also, why is the tool in llvm/utils but the tests in llvm/test/tools?)

I am not sure if there was any test for the utilities from llvm/utils, so we decided to put the tests inside llvm/test/tools directory. Since this is more like a real tool, it may make sense to move the src code of the llvm-locstats inside llvm/tools (or even inside llvm/tools/llvm-dwarfdump). Please let me know what you think.