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

[djtodoro]

Summary

Catch that llvm-dwarfdump detected an overflow in statistics.

[StephenTozer]

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?

[djtodoro]

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

• adding todo marker for handling other errs from llvm-dwarfdump

[djtodoro]

• parse special value for the fields that overflow

[dblaikie]

llvm/utils/llvm-locstats/llvm-locstats.py

199–201

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

[djtodoro]

llvm/utils/llvm-locstats/llvm-locstats.py

199–201

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]

llvm/utils/llvm-locstats/llvm-locstats.py

199–201

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]

llvm/utils/llvm-locstats/llvm-locstats.py

199–201

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]

• introduce special "tainted" value for stats that overflowed

[djtodoro]

I guess this is ok to go as well.

[djtodoro]

ping

[djtodoro]

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

[dblaikie]

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.

[djtodoro]

Thanks!

[thakis]

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.

[djtodoro]

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?

[thakis]

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

[djtodoro]

In D110621#3090251, @thakis wrote:

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.

llvm/test/tools/llvm-dwarfdump/X86/locstats-bytes-overflow.yaml

-# RUN: yaml2obj %s | llvm-dwarfdump --statistics - 2>&1 | FileCheck %s
+# RUN: yaml2obj %s -o %t.o
+# RUN: llvm-dwarfdump --statistics %t.o 2>&1 | FileCheck %s
+# RUN: %llvm-locstats %t.o 2>&1 | FileCheck %s --check-prefix=LOCSTATS
+# RUN: rm %t.o
 
 ## Check that we are covering the situation when a stat field overflows.
 ##
 ## The yaml represents this DWARF:
 ##
 ## DW_TAG_compile_unit
 ##   DW_AT_low_pc (0x0000000000000000)
 ##   DW_AT_high_pc  (0x000000000000000b)
 ##
 ##   DW_TAG_subprogram
 ##     DW_AT_low_pc (0x0000000000000000)
 ##     DW_AT_high_pc  (0xffffffffffffffff)
 ##       DW_TAG_variable
 ##         DW_AT_location (0x00000023:
 ##            [0x0000000000000003, 0x0000000000000005): DW_OP_reg2 RCX)
 ##   DW_TAG_subprogram
 ##     DW_AT_low_pc    (0x0000000000000000)
 ##     DW_AT_high_pc   (0xffffffffffffffff)
 ##     DW_TAG_variable
 ##       DW_AT_location        (0x00000023:
 ##             [0x0000000000000003, 0x0000000000000005): DW_OP_reg2 RCX)
 
 # CHECK: warning:  this field overflows
+# LOCSTATS: warning: There is an overflow detected by llvm-dwarfdump backend.
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS[[BITS=64]]
   Data:    ELFDATA2LSB
   Type:    ET_EXEC
   Machine: EM_X86_64
 Sections:

llvm/utils/llvm-locstats/llvm-locstats.py

def parse_locstats(opts, binary):
   llvm_dwarfdump_stats_opt = "--statistics"
 
   # Generate the stats with the llvm-dwarfdump.
   subproc = Popen([llvm_dwarfdump_cmd, llvm_dwarfdump_stats_opt, binary], \
                   stdin=PIPE, stdout=PIPE, stderr=PIPE, \
                   universal_newlines = True)
   cmd_stdout, cmd_stderr = subproc.communicate()
 
+  # TODO: Handle other errors that are coming from llvm-dwarfdump.
+
+  if "field overflows" in cmd_stderr:
+    print('warning: There is an overflow detected by llvm-dwarfdump backend.')
+
   # Get the JSON and parse it.
   json_parsed = None
 
   try:
     json_parsed = loads(cmd_stdout)
   except:
     print ('error: No valid llvm-dwarfdump statistics found.')
     sys.exit(1)
 
   # TODO: Parse the statistics Version from JSON.
 
   if opts.only_variables:
     # Read the JSON only for local variables.
     variables_total_locstats = \
       json_parsed['#local vars processed by location statistics']
     variables_scope_bytes_covered = \
       json_parsed['sum_all_local_vars(#bytes in parent scope covered' \
                   ' by DW_AT_location)']
     variables_scope_bytes = \
       json_parsed['sum_all_local_vars(#bytes in parent scope)']
     if not opts.ignore_debug_entry_values:
       for cov_bucket in coverage_buckets():
         cov_category = "#local vars with {} of parent scope covered " \