Page MenuHomePhabricator

[clangd] A tool to evaluate cross-file rename.
Needs ReviewPublic

Authored by hokein on Dec 6 2019, 3:55 AM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

This is a tool that helps us to evaluate the rename results, we may not
submit this tool to the code repository, but it has value, helps us find
out bugs in AST/Refs/Rename code.

Diff Detail

Unit TestsFailed

TimeTest
10 msLLVM.Bindings/Go::go.test
Script: -- : 'RUN: at line 1'; llvm-go test llvm.org/llvm/bindings/go/llvm

Event Timeline

hokein created this revision.Dec 6 2019, 3:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 6 2019, 3:55 AM

Build result: pass - 60496 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt, CMakeCache.txt

hokein updated this revision to Diff 232553.Dec 6 2019, 7:08 AM

add more symbols.

hokein updated this revision to Diff 232554.Dec 6 2019, 7:10 AM

Fix empty lines.

hokein added a subscriber: kbobyrev.Dec 6 2019, 7:11 AM

Build result: pass - 60496 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt, CMakeCache.txt

Build result: pass - 60496 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt, CMakeCache.txt

kbobyrev added a comment.EditedDec 31 2019, 6:40 AM

For some reason, running this patch on a recent version of the source tree fails. The AST parsing fails with fatal diagnostics, but I can't understand why:

E[15:14:33.084] source file unknown type name 'PathRef' is illformed: 1
E[15:15:02.831] source file unknown type name 'NestedNameSpecifierLoc'; did you mean 'std::clang::NestedNameSpecifierLoc'? is illformed: 1
E[15:14:02.912] source file unknown type name 'Location' is illformed: 1
E[15:14:47.797] source file 'index' is not a class, namespace, or enumeration is illformed: 1
E[15:14:17.658] source file no template named 'vector' in namespace 'std' is illformed: 1

I've had many unsuccessful attempts to track down the bug and use multiple different configurations (i.e. I thought something might be wrong with my index location or directory structure, so in the end I replicated the setup that I can infer from the code), but I still didn't manage to run the renames.

In order to make it easier for you to understand what went wrong I have logged my commands and output so that one could understand what happens. I've tried several versions of LLVM, the log is for the most recent one, https://github.com/kirillbobyrev/llvm-project/commit/c7dc4734d23f45f576ba5af2aae5be9dfa2d3643. I've made sure to run check-all to validate correctness of source tree and to generate all test files so that indexer does not crash. I've made sure that all the commands ran without any errors.

Because the output is quite verbose, I'm submitting another file which only lists the commands instead of the one with verbose output. If you need the full log, please let me know. Attached file:

Please let me know if I have done something incorrectly.

clang-tools-extra/clangd/eval-rename/RenameMain.cpp
35

Maybe just Apply? Fix seems slightly confusing.

clang-tools-extra/clangd/eval-rename/eval-rename.py
2

It probably makes sense to migrate this to Python 3 instead (shouldn't be too hard, from what I can see there are Python 2 print statements, but nothing else I can find).

12

Maybe some comments regarding what each field in symbol_to_rename.txt corresponds to and how it is parsed would be useful.

22

Would be useful to add an argument to the build directory, I typically don't put build/ into llvm/.

24

nit: run?

83

git reset --hard might be dangerous if compile-commands are symlinked to the build directory: ninja -C build would re-generate them and git reset --hard will e.g. erase add_subdirectory(eval-rename) if it's not committed. Maybe this should be mentioned somewhere in the comments/user guide.

hokein updated this revision to Diff 243768.Tue, Feb 11, 1:42 AM
hokein marked 3 inline comments as done.
  • rebase to master
  • fix the breakage of the rename tool
  • add more symbol to the tests
  • some tweaks on the python script

sorry for the delay, I just saw the comment today.

For some reason, running this patch on a recent version of the source tree fails. The AST parsing fails with fatal diagnostics, but I can't understand why:

E[15:14:33.084] source file unknown type name 'PathRef' is illformed: 1
E[15:15:02.831] source file unknown type name 'NestedNameSpecifierLoc'; did you mean 'std::clang::NestedNameSpecifierLoc'? is illformed: 1
E[15:14:02.912] source file unknown type name 'Location' is illformed: 1
E[15:14:47.797] source file 'index' is not a class, namespace, or enumeration is illformed: 1
E[15:14:17.658] source file no template named 'vector' in namespace 'std' is illformed: 1

I encountered the same error when rebasing to master. It was work before. To parse the file correctly, we need to inject the clang builtin header directory (via resource-dir) to the compile command, it was done by OverlayCDB, but there was a patch changing this behavior in December, which broke the tool.
It should be fixed now.

clang-tools-extra/clangd/eval-rename/eval-rename.py
2

good point.

22

yeah. the current script currently assumes the directory layout:

llvm-project
  - build/
    - bin/...
  - clang-tools-extra
  - ...
83

agree. it is dangerous, the script is expected to be ran on a clean client. we could improve it, e.g. prompt a confirm dialog, or abandon if there are dirty changes in the client.

Thanks! Now I can build it.

I was able to get some rename failures through this tool, though :( I had to disable the check for number of affected files first, but then this seems to be problematic:

llvm/include/llvm/ADT/ArrayRef.h llvm::ArrayRef clangd
llvm/include/llvm/ADT/StringRef.h llvm::StringRef clangd
kbobyrev added a comment.EditedWed, Feb 12, 3:59 AM

Renaming llvm::Optional also fails (clangd-rename says the symbol is not of a supported kind even though it's a class).

Renaming llvm::None is also not working: there seem to be many problems but a lot of them are connected with function's argument default values (void foo(llvm::Optional<int> Bar = None).

llvm/include/llvm/ADT/Optional.h llvm::Optional clangd
llvm/include/llvm/ADT/None.h llvm::None clangd

Thanks for the feedback!

Yeah, currently template classes are not supported in cross-file rename, see https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clangd/refactor/Rename.cpp#L187.

So llvm::Optional and llvm::Optional should fail to rename, but StringRef and llvm::None should work (if you remove the hard-coded max limit 50).

Thanks for the feedback!

Yeah, currently template classes are not supported in cross-file rename, see https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clangd/refactor/Rename.cpp#L187.

So llvm::Optional and llvm::Optional should fail to rename, but StringRef and llvm::None should work (if you remove the hard-coded max limit 50).

Ah, I see, thank you for the explanation! I think it would be super useful to have the error message reflecting that!

Also, the llvm::Twine fails too, many of the failures are related to the forward declarations (I think the problem is that they are treated differently from the original symbol and are not being renamed at all in the corresponding files, but that's just my guess). It`s probably a good idea to keep track of all the entries that can break global rename in some issue on the Github so that they're not lost?

Also, the llvm::Twine fails too, many of the failures are related to the forward declarations (I think the problem is that they are treated differently from the original symbol and are not being renamed at all in the corresponding files, but that's just my guess). It`s probably a good idea to keep track of all the entries that can break global rename in some issue on the Github so that they're not lost?

Yeap, that sounds good to me. Just create an issue, and put all details there, attach the detailed log would be helpful. I suspect we may encounter bugs in xrefs/rename code.