[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

218–220

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

[djtodoro]

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]

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]

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]

• 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: "sum_all_variables(#bytes in parent scope)": "overflowed"
+# LOCSTATS: warning: "sum_all_variables(#bytes in parent scope)" field overflowed.
+# LOCSTATS: PC ranges covered: tainted
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS[[BITS=64]]
   Data:    ELFDATA2LSB
   Type:    ET_EXEC
   Machine: EM_X86_64
 Sections:

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

 #!/usr/bin/env python
 #
 # This is a tool that works like debug location coverage calculator.
 # It parses the llvm-dwarfdump --statistics output by reporting it
 # in a more human readable way.
 #
 
 from __future__ import print_function
 import argparse
 import os
 import sys
 from json import loads
 from math import ceil
 from collections import OrderedDict
 from subprocess import Popen, PIPE
 
+# This special value has been used to mark statistics that overflowed.
+TAINT_VALUE = "tainted"
+
 # Initialize the plot.
 def init_plot(plt):
   plt.title('Debug Location Statistics', fontweight='bold')
   plt.xlabel('location buckets')
   plt.ylabel('number of variables in the location buckets')
   plt.xticks(rotation=45, fontsize='x-small')
   plt.yticks()
 
def __init__(self, file_name, variables_total, variables_total_locstats,
     self.variables_total_locstats = variables_total_locstats
     self.variables_with_loc = variables_with_loc
     self.scope_bytes_covered = variables_scope_bytes_covered
     self.scope_bytes = variables_scope_bytes
     self.variables_coverage_map = variables_coverage_map
 
   # Get the PC ranges coverage.
   def get_pc_coverage(self):
+    if self.scope_bytes_covered == TAINT_VALUE or \
+       self.scope_bytes == TAINT_VALUE:
+      return TAINT_VALUE
     pc_ranges_covered = int(ceil(self.scope_bytes_covered * 100.0) \
                 / self.scope_bytes)
     return pc_ranges_covered
 
   # Pretty print the debug location buckets.
   def pretty_print(self):
     if self.scope_bytes == 0:
       print ('No scope bytes found.')
       return -1
 
     pc_ranges_covered = self.get_pc_coverage()
     variables_coverage_per_map = {}
     for cov_bucket in coverage_buckets():
+      variables_coverage_per_map[cov_bucket] = None
+      if self.variables_coverage_map[cov_bucket] == TAINT_VALUE or \
+         self.variables_total_locstats == TAINT_VALUE:
+        variables_coverage_per_map[cov_bucket] = TAINT_VALUE
+      else:
         variables_coverage_per_map[cov_bucket] = \
           int(ceil(self.variables_coverage_map[cov_bucket] * 100.0) \
                    / self.variables_total_locstats)
 
     print (' =================================================')
     print ('            Debug Location Statistics       ')
     print (' =================================================')
     print ('     cov%           samples         percentage(~)  ')
     print (' -------------------------------------------------')
     for cov_bucket in coverage_buckets():
+      if self.variables_coverage_map[cov_bucket] or \
+         self.variables_total_locstats == TAINT_VALUE:
+        print ('   {0:10}     {1:8}              {2:3}%'. \
+          format(cov_bucket, self.variables_coverage_map[cov_bucket], \
+                 variables_coverage_per_map[cov_bucket]))
+      else:
         print ('   {0:10}     {1:8d}              {2:3d}%'. \
           format(cov_bucket, self.variables_coverage_map[cov_bucket], \
                  variables_coverage_per_map[cov_bucket]))
     print (' =================================================')
     print (' -the number of debug variables processed: ' \
       + str(self.variables_total_locstats))
     print (' -PC ranges covered: ' + str(pc_ranges_covered) + '%')
 
     # Only if we are processing all the variables output the total
     # availability.
     if self.variables_total and self.variables_with_loc:
+      total_availability = None
+      if self.variables_total == TAINT_VALUE or \
+         self.variables_with_loc == TAINT_VALUE:
+        total_availability = TAINT_VALUE
+      else:
         total_availability = int(ceil(self.variables_with_loc * 100.0) \
                                       / self.variables_total)
       print (' -------------------------------------------------')
       print (' -total availability: ' + str(total_availability) + '%')
     print (' =================================================')
 
     return 0
 
   # Draw a plot representing the location buckets.
   def draw_plot(self):
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 errors that are coming from llvm-dwarfdump.
+
   # 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.
 
+  def init_field(name):
+    if json_parsed[name] == 'overflowed':
+      print ('warning: "' + name + '" field overflowed.')
+      return TAINT_VALUE
+    return json_parsed[name]
+
   if opts.only_variables:
     # Read the JSON only for local variables.
     variables_total_locstats = \
-      json_parsed['#local vars processed by location statistics']
+      init_field('#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)']
+      init_field('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)']
+      init_field('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 " \
                        "by DW_AT_location".format(cov_bucket)
-        variables_coverage_map[cov_bucket] = json_parsed[cov_category]
+        variables_coverage_map[cov_bucket] = init_field(cov_category)
     else:
       variables_scope_bytes_entry_values = \
-        json_parsed['sum_all_local_vars(#bytes in parent scope ' \
-                    'covered by DW_OP_entry_value)']
+        init_field('sum_all_local_vars(#bytes in parent scope ' \
+                    'covered by DW_OP_entry_value)')
+      if variables_scope_bytes_covered != TAINT_VALUE and \
+         variables_scope_bytes_entry_values != TAINT_VALUE:
         variables_scope_bytes_covered = variables_scope_bytes_covered \
            - variables_scope_bytes_entry_values
       for cov_bucket in coverage_buckets():
         cov_category = \
           "#local vars - entry values with {} of parent scope " \
           "covered by DW_AT_location".format(cov_bucket)
-        variables_coverage_map[cov_bucket] = json_parsed[cov_category]
+        variables_coverage_map[cov_bucket] = init_field(cov_category)
   elif opts.only_formal_parameters:
     # Read the JSON only for formal parameters.
     variables_total_locstats = \
-      json_parsed['#params processed by location statistics']
+      init_field('#params processed by location statistics')
     variables_scope_bytes_covered = \
-      json_parsed['sum_all_params(#bytes in parent scope covered ' \
-                  'by DW_AT_location)']
+      init_field('sum_all_params(#bytes in parent scope covered ' \
+                  'by DW_AT_location)')
     variables_scope_bytes = \
-      json_parsed['sum_all_params(#bytes in parent scope)']
+      init_field('sum_all_params(#bytes in parent scope)')
     if not opts.ignore_debug_entry_values:
       for cov_bucket in coverage_buckets():
         cov_category = "#params with {} of parent scope covered " \
                        "by DW_AT_location".format(cov_bucket)
-        variables_coverage_map[cov_bucket] = json_parsed[cov_category]
+        variables_coverage_map[cov_bucket] = init_field(cov_category)
     else:
       variables_scope_bytes_entry_values = \
-        json_parsed['sum_all_params(#bytes in parent scope covered ' \
-                    'by DW_OP_entry_value)']
+        init_field('sum_all_params(#bytes in parent scope covered ' \
+                    'by DW_OP_entry_value)')
+      if variables_scope_bytes_covered != TAINT_VALUE and \
+         variables_scope_bytes_entry_values != TAINT_VALUE:
         variables_scope_bytes_covered = variables_scope_bytes_covered \
           - variables_scope_bytes_entry_values
       for cov_bucket in coverage_buckets():
         cov_category = \
           "#params - entry values with {} of parent scope covered" \
           " by DW_AT_location".format(cov_bucket)
-        variables_coverage_map[cov_bucket] = json_parsed[cov_category]
+        variables_coverage_map[cov_bucket] = init_field(cov_category)
   else:
     # Read the JSON for both local variables and formal parameters.
     variables_total = \
-      json_parsed['#source variables']
-    variables_with_loc = json_parsed['#source variables with location']
+      init_field('#source variables')
+    variables_with_loc = init_field('#source variables with location')
     variables_total_locstats = \
-      json_parsed['#variables processed by location statistics']
+      init_field('#variables processed by location statistics')
     variables_scope_bytes_covered = \
-      json_parsed['sum_all_variables(#bytes in parent scope covered ' \
-                  'by DW_AT_location)']
+      init_field('sum_all_variables(#bytes in parent scope covered ' \
+                  'by DW_AT_location)')
     variables_scope_bytes = \
-      json_parsed['sum_all_variables(#bytes in parent scope)']
+      init_field('sum_all_variables(#bytes in parent scope)')
+
     if not opts.ignore_debug_entry_values:
       for cov_bucket in coverage_buckets():
         cov_category = "#variables with {} of parent scope covered " \
                        "by DW_AT_location".format(cov_bucket)
-        variables_coverage_map[cov_bucket] = json_parsed[cov_category]
+        variables_coverage_map[cov_bucket] = init_field(cov_category)
     else:
       variables_scope_bytes_entry_values = \
-        json_parsed['sum_all_variables(#bytes in parent scope covered ' \
-                    'by DW_OP_entry_value)']
+        init_field('sum_all_variables(#bytes in parent scope covered ' \
+                    'by DW_OP_entry_value)')
+      if variables_scope_bytes_covered != TAINT_VALUE and \
+         variables_scope_bytes_entry_values != TAINT_VALUE:
         variables_scope_bytes_covered = variables_scope_bytes_covered \
           - variables_scope_bytes_entry_values
       for cov_bucket in coverage_buckets():
         cov_category = \
           "#variables - entry values with {} of parent scope covered " \
           "by DW_AT_location".format(cov_bucket)
-        variables_coverage_map[cov_bucket] = json_parsed[cov_category]
+        variables_coverage_map[cov_bucket] = init_field(cov_category)
 
   return LocationStats(binary, variables_total, variables_total_locstats,
                        variables_with_loc, variables_scope_bytes_covered,
                        variables_scope_bytes, variables_coverage_map)
 
 # Parse the program arguments.
 def parse_program_args(parser):
   parser.add_argument('--only-variables', action='store_true', default=False,