[RemoveDIs] Print IR with debug records by default

[SLTozer]

This patch makes the final major change of the RemoveDIs project, changing the default IR output from debug intrinsics to debug records.

This is expected to break a large number of tests: every single one that tests for uses or declarations of debug intrinsics and does not explicitly disable writing records. There is also some collateral damage for tests that merely contains debug intrinsic declarations and tests for attributes, as debug intrinsic declarations will now be deleted when writing debug records which may result in renumbering of other attributes.

Review guidance

This patch is divided into distinct sections, in the following order and comprising the following commits:

1. NFC manual test updates in preparation for the automated test updates.
   • Pre-script fixups
2. The functional change of the patch, which enables the printing of debug records by default.
   • Print debug records by default
3. The automated test updates - I recommend not reviewing the contents of this patch, but reviewing the scripts as explained in the section below.
   • Update test checks
   • update_cc_test_checks
   • Script update manual tests
4. Further manual test updates to fix up test errors that cannot be fixed by (or are caused by) the automated test updates.
   • Manual fixups post-update_test_checks

Update script details

For the most part, replacing debug intrinsics with debug records is trivial string substitution; replacing FileCheck CHECK lines for debug intrinsics is a slightly more complicated string substitution. For tests that use update_test_checks.py, updating the tests is entirely automatic, but for the many debug info tests that do not use update_test_checks.py I've used a less "smart" approach. The steps to produce the test updates are as follow:

1. Collect the list of failing lit tests into a single file, failing-tests.txt, separated by newlines.
2. Use the following line to split the failing tests into tests that use update_test_checks and tests that don't:

   $ while IFS= read -r f; do grep -q "Assertions have been autogenerated by utils/update_test_checks.py" "$f" && echo "$f" >> update-checks-tests.txt || echo "$f" >> manual-tests.txt; done < failing-tests.txt
   

3. For the tests that use update_test_checks, run the appropriate update_test_checks script - for the updates run here, this was achieved with:

   $ xargs ./llvm/utils/update_test_checks.py --opt-binary ./build/bin/opt < update-checks-tests.txt
   $ xargs ./llvm/utils/update_cc_test_checks.py --llvm-bin ./build/bin/ < update-checks-tests.txt
   

   As far as I could tell, there was no way to get the llvm and clang tests invoked with a single tool - update_test_checks.py does not accept an --llvm-bin argument, which means it can't find the right clang by itself, so the second call is needed. There may be additional arguments needed depending on which tools are being tested; all the applicable tests I encountered were satisfied with just the above, but it's possible the tests will need to be further split if some depend on other tools - I didn't write any script to handle such cases automatically, since I didn't encounter any.
4. For the manual tests, I used a pair of short scripts to find check-prefixes for the failing tests, and another to update the corresponding check-lines:

   $ cat ./get-checks.sh
   #!/bin/bash
   
   # Always add CHECK, since it's more effort than it's worth to filter files where
   # every RUN line uses other check prefixes.
   # Then detect every instance of "check-prefix(es)=..." and add the
   # comma-separated arguments as extra checks.
   for filename in "$@"
   do
       echo "$filename,CHECK"
       allchecks=$(grep -Eo 'check-prefix(es)?[ =][A-Z0-9_,-]+' $filename | sed -E 's/.+[= ]([A-Z0-9_,-]+).*/\1/g; s/,/\n/g')
       for check in $allchecks; do
           echo "$filename,$check"
       done
   done
   $ cat ./substitute-checks.sh
   #!/bin/bash
   
   file="$1"
   check="$2"
   
   # Any test that explicitly tests debug intrinsic output is not suitable to
   # update by this script.
   if grep -q "write-experimental-debuginfo=false" "$file"; then
       exit 0
   fi
   
   sed -i -E -e "
   /(#|;|\/\/).*$check[A-Z0-9_\-]*:/!b
   /DIGlobalVariableExpression/b
   /!llvm.dbg./bpostcall
   s/((((((no|must)?tail )?call.*)?void )?@)?llvm.)?dbg\.([a-z]+)/#dbg_\7/
   :postcall
   /declare #dbg_/d
   s/metadata //g
   s/metadata\{/{/g
   s/DIExpression\(([^)]*)\)\)(,( !dbg)?)?/DIExpression(\1),/
   /#dbg_/!b
   s/((\))?(,) )?!dbg (![0-9]+)/\3\4\2/
   s/((\))?(, ))?!dbg/\3/
   " "$file"
   $ cat manual-tests.txt | xargs ./get-checks.sh | sort | uniq | awk -F ',' '{ system("./substitute-checks.sh " $1 " " $2) }'
   

   These rather ugly scripts deal successfully with the vast majority of checks in clang/test and llvm/test; more explanation of why I used these scripts appears below.
5. Verify the resulting tests pass, and detect any failing tests:

   $ xargs ./build/bin/llvm-lit -q < failing-tests.txt
   ********************
   Failed Tests (5):
   LLVM :: DebugInfo/Generic/dbg-value-lower-linenos.ll
   LLVM :: Transforms/HotColdSplit/transfer-debug-info.ll
   LLVM :: Transforms/ObjCARC/basic.ll
   LLVM :: Transforms/ObjCARC/ensure-that-exception-unwind-path-is-visited.ll
   LLVM :: Transforms/SafeStack/X86/debug-loc2.ll
   
   
   Total Discovered Tests: 295
   Failed: 5 (1.69%)
   

6. Some tests may have failed - the update scripts are deliberately not smart, and so there are cases that they will not handle. These must be manually dealt with.

What's with the update scripts?

The test scripts above are deliberately dumb - they only operate on basic string substitution, for two reasons: the test update step should be a one-time process, so putting effort into building a complete tool is likely to be wasted effort when a simple bash script and a small amount of manual editing will suffice; also, being very simple should give reassurance that nothing magical is happening here - the syntax difference between debug intrinsics and debug records is very small, and as long as you know you're processing a debug intrinsic the update rules are incredibly simple. Most of the complexity of the scripts and almost all the error cases result from check-lines which do not obviously apply to a debug intrinsic, and the difficulty of manually fixing up these cases is low enough that a smarter script with a better understanding of context was unlikely to be worthwhile.

How do I fix test failures?

The simplest way to fix a test failure is to not use the new format; if an immediate fix is needed for some set of tests, then adding the LLVM flag --write-experimental-debuginfo=false will force it to write debug intrinsics, preserving prior behaviour. This should only be a temporary fix in most cases though, as we are moving towards deprecating debug intrinsics.

The commits in which I fixed up tests are labelled "Pre-script fixups" and "Post-script fixups" in this review, for examples of changes that were made. The reason these are divided into pre- and post- is that some fixups are trivial text changes to make check-lines intelligible to the sed scripts, and some are changes that are breaking if applied to the debug intrinsic case, so should not apply before the functional change. The pre-script changes are:

• Modifying unusually-structured check-lines to be more standard:

  -  int l = 0;    // line #4: CHECK: {{call.*llvm.dbg.declare.*%l.*\!dbg }}[[variable_l:![0-9]+]]
  +  int l = 0;    // line #4: CHECK: call void @llvm.dbg.declare({{.*%l.*}}!dbg [[variable_l:![0-9]+]]
  

• Changing some over-eager capture patterns:

  -  // CHECK: call void @llvm.dbg.label(metadata [[LABEL_METADATA:!.*]]), !dbg [[LABEL_LOCATION:!.*]]
  +  // CHECK: call void @llvm.dbg.label(metadata [[LABEL_METADATA:!.*]]), !dbg [[LABEL_LOCATION:![0-9]+]]
  

  In the example above, the LABEL_LOCATION variable will just capture "!10" in intrinsic form, but will capture "!10)" in record form since the debug loc now appears in a parenthesis-enclosed list.
• Removing certain !dbg prefixes:

  -// CHECK: call {{.*}}, !dbg ![[DBG_LINE:[0-9]+]]
  +// CHECK: call void @llvm.dbg.declare({{.+}}), !dbg ![[DBG_LINE:[0-9]+]]
  

  Where !dbg !<Num> appears in debug intrinsics, we only see !<Num> in debug records; the script handles this in cases where we are clearly operating on a debug intrinsic, but when the debug location check appears on a separate check-line it must be manually updated. In some cases this will require some additional context to be added to the check to ensure that the test is still valid.
• Changes without context:

  -  ; CHECK-SAME:               DW_OP_LLVM_convert, 32, DW_ATE_unsigned, DW_OP_stack_value))
  +  ; CHECK-SAME:               DW_OP_LLVM_convert, 32, DW_ATE_unsigned, DW_OP_stack_value)
  

  Some minor differences, like DIExpressions no longer being the final argument to a debug record, can't be fixed by the script because they appear on a line without the necessary context; these must be updated manually.
• Fixing typos:

  -; CHECK: all void @llvm.dbg.value(metadata ptr %lsr.iv, metadata ![[MID_p:[0-9]+]],  metadata !DIExpression(DW_OP_constu, 3, DW_OP_minus, DW_OP_stack_value))
  +; CHECK: call void @llvm.dbg.value(metadata ptr %lsr.iv, metadata ![[MID_p:[0-9]+]],  metadata !DIExpression(DW_OP_constu, 3, DW_OP_minus, DW_OP_stack_value))
  

• Ensuring that the debug intrinsic format is used where necessary:

  -; RUN: llvm-dis < %s.bc | FileCheck %s
  +; RUN: llvm-dis < %s.bc --write-experimental-debuginfo=false | FileCheck %s
  

  Some tests need to continue using the debug intrinsic format; use --write-experimental-debuginfo=false for these cases, and the update script will ignore them.

The post-script changes are generally simpler:

• Update attributes:

  -; CHECK: attributes #1 = { nocallback nofree nosync nounwind speculatable willreturn memory(none) }
  

  Debug intrinsic declarations are being removed, and the resulting attributes may disappear from the output.
• Fix script errors:

  -; CHECK: call void @llvm.random.metadata.use(ptr undef
  +; CHECK: call void @llvm.random.metadata.use(metadata ptr undef
  

  The update script errs on the side of caution, but there are some unusual cases where the patterns it expects to update are over-eager, such as the use of MetadataAsValue in functions other than debug intrinsics.

[OCHyams]

LGTM to post upstream. Some thoughts you (but feel free to ignore):

I think you can drop this entire para, which basically says "might be unknown unknowns" (can be shortened if you think it still needs to be said IMO - it's already a long read):

As far as I could tell, there was no way to get the llvm and clang tests invoked with a single tool - update_test_checks.py does not accept an --llvm-bin argument, which means it can't find the right clang by itself, so the second call is needed. There may be additional arguments needed depending on which tools are being tested; all the applicable tests I encountered were satisfied with just the above, but it's possible the tests will need to be further split if some depend on other tools - I didn't write any script to handle such cases automatically, since I didn't encounter any.

I think it'd improve readability to split the code in step 4 into three code blocks, one for each small script and one for the run line.

The test scripts above are deliberately dumb

"deliberately simple"

Are you able to reduce "What's with the update scripts?" section? It seems like the core message is "the scripts are simple because the edge cases are not worth the complexity + scripts are easier to review".

[OCHyams]

What's this pre-script fixup about?

[SLTozer]

Removing an overly aggressive capture group, which causes problems when BEGIN1 can match multiple things (either the variable or the location).

[OCHyams]

Is it worth adding comments to these tests explaining why new intrinsics are not used?