RefactoringTool does not rewrite sources in another directory

[Quuxplusone]

Bugzilla Link	PR22385
Status	CONFIRMED
Importance	P normal
Reported by	Tim Besard (tim.besard@elis.ugent.be)
Reported on	2015-01-29 08:09:57 -0800
Last modified on	2015-09-02 02:38:30 -0700
Version	3.6
Hardware	PC Linux
CC	alexfh@google.com, guillaume.papin@epitech.eu, hans@chromium.org, klimek@google.com, llvm-bugs@lists.llvm.org
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

If I use the RefactoringTool to match and rewrite files which reside in a
subdirectory, matching works fine, but Rewriting fails because the sources are
not found anymore (the directory component is stripped off).

For example, demonstrating using remove-cstr-calls from clang-tools-extra, if I
have a file main.cpp in /tmp/test/src, and an accompanying
compile_database.json (in the same directory) looking as follows:
[
    {
        "command": "g++ -c -o main.o main.cpp",
        "directory": "/tmp/test/src",
        "file": "/tmp/test/src/main.cpp"
    }
]
(this compile database is generated using Bear)

If I now call the tooling binary from /tmp/test using `remove-cstr-calls src
src/main.cpp`, I get:

fatal error: cannot open file 'main.cpp': No such file or directory
remove-cstr-calls:
../../tools/clang/include/clang/Rewrite/Core/RewriteRope.h:203: void
clang::RewriteRope::erase(unsigned int, unsigned int): Assertion
`Offset+NumBytes <= size() && "Invalid region to erase!"' failed.
#0 0x7fc86c9a9c48 llvm::sys::PrintStackTrace(_IO_FILE*)
../../lib/Support/Unix/Signals.inc:422:15
#1 0x7fc86c9ab24b SignalHandler(int) ../../lib/Support/Unix/Signals.inc:198:28
#2 0x7fc86a7b4b20 __restore_rt (/usr/lib/libc.so.6+0x33b20)
#3 0x7fc86a7b4a97 __GI_raise (/usr/lib/libc.so.6+0x33a97)
#4 0x7fc86a7b5e6a __GI_abort (/usr/lib/libc.so.6+0x34e6a)
#5 0x7fc86a7ad8bd __assert_fail_base (/usr/lib/libc.so.6+0x2c8bd)
#6 0x7fc86a7ad972 (/usr/lib/libc.so.6+0x2c972)
#7 0x7fc86798f7ce (bin/../lib/../lib/libclangRewrite.so+0x87ce)
#8 0x7fc867990788 clang::Rewriter::ReplaceText(clang::SourceLocation, unsigned
int, llvm::StringRef) ../../tools/clang/lib/Rewrite/Rewriter.cpp:311:3
#9 0x7fc86b35105d clang::tooling::Replacement::apply(clang::Rewriter&) const
../../tools/clang/lib/Tooling/Core/Replacement.cpp:73:28
#10 0x7fc86b351a3e
clang::tooling::applyAllReplacements(std::set<clang::tooling::Replacement,
std::less<clang::tooling::Replacement>,
std::allocator<clang::tooling::Replacement> > const&, clang::Rewriter&)
../../tools/clang/lib/Tooling/Core/Replacement.cpp:234:16
#11 0x7fc86b567898
clang::tooling::RefactoringTool::runAndSave(clang::tooling::FrontendActionFactory*)
../../tools/clang/lib/Tooling/Refactoring.cpp:48:8
#12 0x404eed main ../../tools/clang/tools/extra/remove-cstr-
calls/RemoveCStrCalls.cpp:236:10
#13 0x7fc86a7a1040 __libc_start_main (/usr/lib/libc.so.6+0x20040)
#14 0x402f78 _start (bin/remove-cstr-calls+0x402f78)
Aborted (core dumped)

If I call the binary from /tmp/test/src (the directory where main.cpp resides)
using `remove-cstr-calls . main.cpp`, everything works fine and the file is
properly rewritten.

I'm using LLVM/Clang from the 3.6 branch, at revision 227398.

[Quuxplusone]

I've tested this bug with remove-cstr-calls compiled against LLVM/Clang 3.5.1, and everything works as expected (refactoring files in a subdirectory, that is). So this seems like a regression from 3.5 to 3.6.

Some more details, the file I tested to rewrite:

----main.cpp--------------------

include

int main() { std::string Hello = "World"; std::out << std::string(Hello.c_str()) << std::endl;

return 1;

}

I checked out RemoveCstrCalls.cpp from release_35, compiled it against my system 3.5.1 LLVM installation, and called the binary as follows:

in /tmp/test: $ llvm-3.5/remove-cstr-calls src src/main.cpp

This works without error, while using remove-cstr-calls from 3.6 fails.

[Quuxplusone]

I've bisected this to r221600.

[Quuxplusone]

(In reply to comment #2)
> I've bisected this to r221600.

Alex, can you take a look?

[Quuxplusone]

The problem is that the paths in replacements turn out to be relative. That seems strange to me, as Replacement::setFromSourceLocation which seems to be used there, makes paths absolute.

I'll take a look at this tomorrow.

[Quuxplusone]

Has there been any progress on this one?

[Quuxplusone]

Not yet, unfortunately. Trying to switch to this.

[Quuxplusone]

(In reply to comment #6)
> Not yet, unfortunately. Trying to switch to this.

Any update?

I'm not sure if this is worth blocking the release on, but it sounds like it
would be nice to get fixed.

[Quuxplusone]

It seems like Replacement::setFromSourceLocation fails to make the path
absolute (fs::make_absolute fails) because the FileEntry it gets from the
SourceManager does not contain a full path. In case of the test set-up I used
above, the FileEntry contains file="main.cpp" dir="."
I tried to look further but couldn't easily find where the SourceManager is
populated.

[Quuxplusone]

Hans, I think, this shouldn't block the release, as there's an easy workaround (run from the directory where the compilation database points to).

But yeah, it would be nice to fix, and I'm looking into this. BTW, clang-check fails this way when trying to print an error message as well.

[Quuxplusone]

I meant clang-tidy crashes when outputting an error message in this case. And clang-check works fine.

[Quuxplusone]

(In reply to comment #9)
> Hans, I think, this shouldn't block the release, as there's an easy
> workaround (run from the directory where the compilation database points to).

Sounds reasonable.