This is an archive of the discontinued LLVM Phabricator instance.

Include the mangled name in -ast-dump=json
ClosedPublic

Authored by arichardson on Oct 29 2019, 6:54 AM.

Details

Summary

I am planning to use this feature to make update_cc_test_checks.py less fragile
by obtaining the mangled names directly from -ast-dump=json.

This is a series of five commits (but since they are all small I've squashed them into one review):


Add a --update flag to gen_ast_dump_json_test.py

This will allow updating the JSON tests for new format changes. Instead of
simply appending the JSON to the input file, the script will now make a
copy of the input file up to the "CHECK lines have been autogenerated"
disclaimer and then append the new JSON.


Parse RUN: line for gen_ast_dump_json_test.py with --update


Infer --filters flags when using gen_ast_dump_json_test.py --update


Skip manual tests when using gen_ast_dump_json_tests.py --update


Include the mangled name in -ast-dump=json

I am planning to use this feature to make update_cc_test_checks.py less fragile
by obtaining the mangled names directly from -ast-dump=json. Currently,
it uses c-index-test which ignores the -triple=, etc. arguments that are
in the RUN: line and therefore does not generate checks for some targets.

The AST dump tests were updated using the following script:

for src in $LLVM_SRC/clang/test/AST/ast-dump-*-json.*; do
   if ! python $LLVM_SRC/clang/test/AST/gen_ast_dump_json_test.py --clang $LLVM_BUILD/bin/clang --update --source $src; then
       echo "FAILED TO UPDATE $src";
       break;
   fi;
done

Diff Detail

Event Timeline

arichardson created this revision.Oct 29 2019, 6:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 29 2019, 6:54 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I am planning to use this feature to make update_cc_checks.py less fragile by obtaining the mangled names directly from -ast-dump=json.

update_cc_checks.py -> update_cc_test_checks.py

arichardson edited the summary of this revision. (Show Details)Oct 31 2019, 4:17 AM
arichardson edited the summary of this revision. (Show Details)

Ping. Does this seem an acceptable change to the JSON output @aaron.ballman ?

Thank you for your patience with my delayed review, I've been at standards meetings for C and C++ and I'm a bit behind schedule.

The AST dump tests were updated using the following script:

One of the things @rsmith and I were chatting about was modifying this script so that you don't need to pass the location of Clang or duplicate RUN arguments to regenerate the tests. Richard wrote on the mailing list:

I tried using this script to update a json dump test, and it seems to not really work very well:

  1. it's really inconvenient to invoke; you need to pass in a --clang, copy some options out from two different places in the test file (from the RUN line and from a "--filters" line), and guess how to form the proper command line for the tool
  2. it generates a file with the wrong name (adding a -json before the extension)
  3. it doesn't strip out the CHECK lines from the previous run of the tool
  4. it adds in a bunch of trailing whitespace, causing the file to not match the one in the repository

Have you had a chance to look at making this more usable? I think the approach taken by utils/make-ast-dump-check.sh helps a lot: run the test in its normal configuration to generate the output, and then replace FileCheck with a tool that updates the CHECK lines instead of checking them. (That saves you needing to pass in --clang and --opts, at least, and makes it really easy to update a whole bunch of tests at once by running them all with lit.)

It looks like you've hit some of the major points, and so I definitely think this is a big improvement over what we already have. Would you be interested in further modifications to address the concerns Richard brought up? (Do not feel obligated, because this is still a step in the right direction as-is.)

clang/test/AST/gen_ast_dump_json_test.py
3 ↗(On Diff #226893)

Is this change needed?

I've split out the update script changes into D70119 to make this easier to review

This revision is now accepted and ready to land.Nov 14 2019, 12:20 PM
This revision was automatically updated to reflect the committed changes.

@arichardson The modified test is failing on Darwin. Here's the log output:

http://lab.llvm.org:8080/green/job/clang-stage1-cmake-RA-incremental/5694/consoleFull

Can you please address the test failure?