As discussed in http://lists.llvm.org/pipermail/cfe-dev/2019-October/063459.html
the overflow of the souce locations (limited to 2^31 chars) can generate all sorts of
weird things (bogus warnings, hangs, crashes, miscompilation and correct compilation).
In debug mode this assert would fail. So it might be a good start, as in PR42301,
to detect the failure and exit with a proper error message.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| clang/lib/Basic/SourceManager.cpp | ||
|---|---|---|
| 587 | For debug builds, I could not find any other way to not reach an assert failure other than exiting here. Perhaps there is a more llvm specific way to die? llvm_unreachable() ? | |
| clang/lib/Basic/SourceManager.cpp | ||
|---|---|---|
| 587 | There is a similar error err_file_too_large. How is it handled? llvm_unreachable is intended for code that is unreachable: it causes an assertion failure in debug builds and undefined behavior in release builds. | |
| clang/include/clang/Basic/DiagnosticCommonKinds.td | ||
|---|---|---|
| 288 | ... >, DefaultFatal; | |
Comment Actions
- Add ", DefaultFatal";
| clang/lib/Basic/SourceManager.cpp | ||
|---|---|---|
| 587 | The err_file_too_large above in this file allows the function to finish and return further on. Here, we assert false before the message being printed. | |
| clang/lib/Basic/SourceManager.cpp | ||
|---|---|---|
| 587 | I think you should return an invalid file ID (i.e. return FileID(); and propagate the error through the call stack. Clang can be used as a library, so you can't just call exit(), this would terminate the user's program. | |
| clang/lib/Basic/SourceManager.cpp | ||
|---|---|---|
| 587 | It will still reach an false assert in builds that enable them. But in release it linger on and ends with the correct warning. | |
| clang/lib/Basic/SourceManager.cpp | ||
|---|---|---|
| 587 | Has this problem been fixed? | |
Comment Actions
Yes. It does return a non-valid FileID, and in builds without assert you get the expected error message.
Comment Actions
I was asking about "It will still reach an false assert in builds that enable them". Has this been fixed?
| clang/lib/Basic/SourceManager.cpp | ||
|---|---|---|
| 587 | No. Don't intend to fix the in this patch, as they would be failing at the moment anyway for such cases. | |
Comment Actions
This essentially means that if there is a problem related to this change, say something goes wrong after return FileID(); it would be impossible to debug until the corresponding assertions are fixed. Also the patch lacks tests (and adding a test is impossible because it would fail in debug builds).
Comment Actions
Git diff 979ae80af7ec49624b932954d22cb91900f17121 did not send a test as well. Feel free to send me a reasonable sized reproducer, the one I have is about 36MB. Don't think it will be that well received.
Comment Actions
#!/usr/bin/env python3 def gen_100k(): s = '/*' + (' '*95) + '*/\n' with open('inc100k.h', 'wt') as out_f: for _ in range(0, 1000): out_f.write(s) def gen_100M(): s = '#include "inc100k.h"\n' with open('inc100M.h', 'wt') as out_f: for _ in range(0, 1000): out_f.write(s) def gen_main(): s = '#include "inc100M.h"\n' with open('reproducer.c', 'wt') as out_f: for _ in range(0, 25): out_f.write(s) def main(): gen_100k() gen_100M() gen_main() if __name__ == '__main__': main()
Comment Actions
Just wondering, I don't fully understand why is that important from the point I do a return FileID();. The critical error has already been inserted to the error list. clang shall either die due an assert in debug, or with a nice message in release. Unless clang is all broken, and the error message would be corrupted, which I don't believe is the case.
Comment Actions
clang shall either die due an assert in debug [...]
I think I have already mentioned the reasons why I don't think this is acceptable:
- it would break tests, as they are usually run with enabled assertions (and since this diagnostic is quite easy to test, it should be tested)
- it would make the code harder to debug (i.e. if someone finds another problem related to large includes, they would first need to fix the failing assertions before they could start debugging their problem)
IMHO we should discuss this in person, that would be quicker.
Comment Actions
Rebased
Created a early exit when a source location overflow is detected,
avoiding further processing or reaching other assert failures.
For that, it was required to create a 'Failure' kind for includes,
as well add return points along the callstack.
Added test that works in both release and debug modes.
Comment Actions
LGTM, but since we both work at Arm, let's wait a week for other folks to chime in if they have any objections.
... >, DefaultFatal;