[PATCH] D70183: Detect source location overflow due includes

[Diogo N. Sampaio via Phabricator via cfe-commits]

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:
> > > dnsampaio wrote:
> > > > miyuki wrote:
> > > > > dnsampaio wrote:
> > > > > > 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() ?
> > > > > 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.
> > > > 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.
> > > > If `llvm_unreachable` is undefined behavior, then I should stick to exit(1) ?
> > > 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.
> > It will still reach an false assert in builds that enable them. But in release it linger on and ends with the correct warning.
> Has this problem been fixed?
No. Don't intend to fix the in this patch, as they would be failing at the moment anyway for such cases.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70183/new/

https://reviews.llvm.org/D70183