[PATCH] D70183: Detect source location overflow due includes

miyuki accepted this revision. miyuki added a comment. This revision is now accepted and ready to land. LGTM, but since we both work at Arm, let's wait a week for other folks to chime in if they have any objections. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D70183: Detect source location overflow due includes

miyuki added a comment. > 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

[PATCH] D70183: Detect source location overflow due includes

dnsampaio added a comment. 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

[PATCH] D70183: Detect source location overflow due includes

miyuki added a comment. In D70183#1751813 , @dnsampaio wrote: > 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

[PATCH] D70183: Detect source location overflow due includes

dnsampaio added a comment. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D70183: Detect source location overflow due includes

miyuki requested changes to this revision. miyuki added a comment. This revision now requires changes to proceed. 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

[PATCH] D70183: Detect source location overflow due includes

dnsampaio marked an inline comment as done. dnsampaio added inline comments. Comment at: clang/lib/Basic/SourceManager.cpp:587 +Diag.Report(IncludePos, diag::err_include_too_large); +exit(1); + } miyuki wrote: > dnsampaio wrote: > > miyuki wrote: > > >

[PATCH] D70183: Detect source location overflow due includes

miyuki added a comment. In D70183#1751552 , @dnsampaio wrote: > Yes. It does return a non-valid FileID, and in builds without assert you get > the expected error message. I was asking about "It will still reach an false assert in builds that enable

[PATCH] D70183: Detect source location overflow due includes

dnsampaio marked 2 inline comments as done. dnsampaio added a comment. Yes. It does return a non-valid FileID, and in builds without assert you get the expected error message. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70183/new/

[PATCH] D70183: Detect source location overflow due includes

miyuki added inline comments. Comment at: clang/lib/Basic/SourceManager.cpp:587 +Diag.Report(IncludePos, diag::err_include_too_large); +exit(1); + } dnsampaio wrote: > miyuki wrote: > > dnsampaio wrote: > > > miyuki wrote: > > > > dnsampaio wrote: > > >

[PATCH] D70183: Detect source location overflow due includes

dnsampaio marked an inline comment as done. dnsampaio added a comment. Ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70183/new/ https://reviews.llvm.org/D70183 ___ cfe-commits mailing list

[PATCH] D70183: Detect source location overflow due includes

dnsampaio marked an inline comment as done. dnsampaio added inline comments. Comment at: clang/lib/Basic/SourceManager.cpp:587 +Diag.Report(IncludePos, diag::err_include_too_large); +exit(1); + } miyuki wrote: > dnsampaio wrote: > > miyuki wrote: > > >

[PATCH] D70183: Detect source location overflow due includes

dnsampaio updated this revision to Diff 229254. dnsampaio marked an inline comment as done. dnsampaio added a comment. - Return an invalid FileID instead of exiting. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70183/new/

[PATCH] D70183: Detect source location overflow due includes

miyuki added inline comments. Comment at: clang/lib/Basic/SourceManager.cpp:587 +Diag.Report(IncludePos, diag::err_include_too_large); +exit(1); + } dnsampaio wrote: > miyuki wrote: > > dnsampaio wrote: > > > For debug builds, I could not find any other

[PATCH] D70183: Detect source location overflow due includes

dnsampaio added inline comments. Comment at: clang/lib/Basic/SourceManager.cpp:587 +Diag.Report(IncludePos, diag::err_include_too_large); +exit(1); + } miyuki wrote: > dnsampaio wrote: > > For debug builds, I could not find any other way to not reach an

[PATCH] D70183: Detect source location overflow due includes

dnsampaio updated this revision to Diff 229113. dnsampaio marked 2 inline comments as done. dnsampaio added a comment. - Add ", DefaultFatal"; Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70183/new/ https://reviews.llvm.org/D70183 Files:

[PATCH] D70183: Detect source location overflow due includes

miyuki added inline comments. Comment at: clang/include/clang/Basic/DiagnosticCommonKinds.td:288 + " Clang to process. This may by a result from multiple" + " inclusions of unguarded header files.">; def err_unsupported_bom : Error<"%0 byte order mark detected in '%1', but "

[PATCH] D70183: Detect source location overflow due includes

miyuki added inline comments. Comment at: clang/lib/Basic/SourceManager.cpp:587 +Diag.Report(IncludePos, diag::err_include_too_large); +exit(1); + } dnsampaio wrote: > For debug builds, I could not find any other way to not reach an assert > failure

[PATCH] D70183: Detect source location overflow due includes

dnsampaio created this revision. dnsampaio added reviewers: rsmith, thakis, miyuki. Herald added a project: clang. Herald added a subscriber: cfe-commits. dnsampaio marked an inline comment as done. dnsampaio added inline comments. Comment at:

[PATCH] D70183: Detect source location overflow due includes

dnsampaio marked an inline comment as done. dnsampaio added inline comments. Comment at: clang/lib/Basic/SourceManager.cpp:587 +Diag.Report(IncludePos, diag::err_include_too_large); +exit(1); + } For debug builds, I could not find any other way to not