[PATCH] D70183: Detect source location overflow due includes

[Mikhail Maltsev via Phabricator via cfe-commits]

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
  https://reviews.llvm.org/D70183/new/

https://reviews.llvm.org/D70183



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70183: Detect source location overflow due includes

[Mikhail Maltsev via Phabricator via cfe-commits]

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 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70183



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70183: Detect source location overflow due includes

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

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 all broken, and the error message would be 
corrupted, which I don't believe is the case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70183



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70183: Detect source location overflow due includes

[Mikhail Maltsev via Phabricator via cfe-commits]

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 that well received.




  #!/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()


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70183



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70183: Detect source location overflow due includes

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

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 ACTION
  https://reviews.llvm.org/D70183/new/

https://reviews.llvm.org/D70183



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70183: Detect source location overflow due includes

[Mikhail Maltsev via Phabricator via cfe-commits]

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 assertions are fixed. Also the patch lacks tests (and 
adding a test is impossible because it would fail in debug builds).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70183



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70183: Detect source location overflow due includes

[Mikhail Maltsev via Phabricator via cfe-commits]

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 
them". Has this been fixed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70183



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70183: Detect source location overflow due includes

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

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/

https://reviews.llvm.org/D70183



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70183: Detect source location overflow due includes

[Mikhail Maltsev via Phabricator via cfe-commits]

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:
> > > > > 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70183



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 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
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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:
> > > > 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70183



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70183: Detect source location overflow due includes

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

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/

https://reviews.llvm.org/D70183

Files:
  clang/include/clang/Basic/DiagnosticCommonKinds.td
  clang/lib/Basic/SourceManager.cpp


Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -577,13 +577,18 @@
 SLocEntryLoaded[Index] = true;
 return FileID::get(LoadedID);
   }
+  unsigned FileSize = File->getSize();
+  if (!(NextLocalOffset + FileSize + 1 > NextLocalOffset &&
+NextLocalOffset + FileSize + 1 <= CurrentLoadedOffset)) {
+// From this point, there is no sensible way to point to the current
+// source-location and say: This include at line ### generates a too
+// big file, as the IncludePos received is
+Diag.Report(IncludePos, diag::err_include_too_large);
+return FileID();
+  }
   LocalSLocEntryTable.push_back(
   SLocEntry::get(NextLocalOffset,
  FileInfo::get(IncludePos, File, FileCharacter, 
Filename)));
-  unsigned FileSize = File->getSize();
-  assert(NextLocalOffset + FileSize + 1 > NextLocalOffset &&
- NextLocalOffset + FileSize + 1 <= CurrentLoadedOffset &&
- "Ran out of source locations!");
   // We do a +1 here because we want a SourceLocation that means "the end of 
the
   // file", e.g. for the "no newline at the end of the file" diagnostic.
   NextLocalOffset += FileSize + 1;
Index: clang/include/clang/Basic/DiagnosticCommonKinds.td
===
--- clang/include/clang/Basic/DiagnosticCommonKinds.td
+++ clang/include/clang/Basic/DiagnosticCommonKinds.td
@@ -282,6 +282,10 @@
   "file '%0' modified since it was first processed">, DefaultFatal;
 def err_file_too_large : Error<
   "sorry, unsupported: file '%0' is too large for Clang to process">;
+def err_include_too_large : Error<
+  "sorry, this include generates a translation unit too large for"
+  " Clang to process. This may by a result from multiple"
+  " inclusions of unguarded header files.">, DefaultFatal;
 def err_unsupported_bom : Error<"%0 byte order mark detected in '%1', but "
   "encoding is not supported">, DefaultFatal;
 def err_unable_to_rename_temp : Error<


Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -577,13 +577,18 @@
 SLocEntryLoaded[Index] = true;
 return FileID::get(LoadedID);
   }
+  unsigned FileSize = File->getSize();
+  if (!(NextLocalOffset + FileSize + 1 > NextLocalOffset &&
+NextLocalOffset + FileSize + 1 <= CurrentLoadedOffset)) {
+// From this point, there is no sensible way to point to the current
+// source-location and say: This include at line ### generates a too
+// big file, as the IncludePos received is
+Diag.Report(IncludePos, diag::err_include_too_large);
+return FileID();
+  }
   LocalSLocEntryTable.push_back(
   SLocEntry::get(NextLocalOffset,
  FileInfo::get(IncludePos, File, FileCharacter, Filename)));
-  unsigned FileSize = File->getSize();
-  assert(NextLocalOffset + FileSize + 1 > NextLocalOffset &&
- NextLocalOffset + FileSize + 1 <= CurrentLoadedOffset &&
- "Ran out of source locations!");
   // We do a +1 here because we want a SourceLocation that means "the end of the
   // file", e.g. for the "no newline at the end of the file" diagnostic.
   NextLocalOffset += FileSize + 1;
Index: clang/include/clang/Basic/DiagnosticCommonKinds.td
===
--- clang/include/clang/Basic/DiagnosticCommonKinds.td
+++ clang/include/clang/Basic/DiagnosticCommonKinds.td
@@ -282,6 +282,10 @@
   "file '%0' modified since it was first processed">, DefaultFatal;
 def err_file_too_large : Error<
   "sorry, unsupported: file '%0' is too large for Clang to process">;
+def err_include_too_large : Error<
+  "sorry, this include generates a translation unit too large for"
+  " Clang to process. This may by a result from multiple"
+  " inclusions of unguarded header files.">, DefaultFatal;
 def err_unsupported_bom : Error<"%0 byte order mark detected in '%1', but "
   "encoding is not supported">, DefaultFatal;
 def err_unable_to_rename_temp : Error<
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70183: Detect source location overflow due includes

[Mikhail Maltsev via Phabricator via cfe-commits]

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 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70183



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70183: Detect source location overflow due includes

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

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 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) ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70183



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70183: Detect source location overflow due includes

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

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:
  clang/include/clang/Basic/DiagnosticCommonKinds.td
  clang/lib/Basic/SourceManager.cpp


Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -577,13 +577,18 @@
 SLocEntryLoaded[Index] = true;
 return FileID::get(LoadedID);
   }
+  unsigned FileSize = File->getSize();
+  if (!(NextLocalOffset + FileSize + 1 > NextLocalOffset &&
+NextLocalOffset + FileSize + 1 <= CurrentLoadedOffset)) {
+// From this point, there is no sensible way to point to the current
+// source-location and say: This include at line ### generates a too
+// big file, as the IncludePos received is
+Diag.Report(IncludePos, diag::err_include_too_large);
+exit(1);
+  }
   LocalSLocEntryTable.push_back(
   SLocEntry::get(NextLocalOffset,
  FileInfo::get(IncludePos, File, FileCharacter, 
Filename)));
-  unsigned FileSize = File->getSize();
-  assert(NextLocalOffset + FileSize + 1 > NextLocalOffset &&
- NextLocalOffset + FileSize + 1 <= CurrentLoadedOffset &&
- "Ran out of source locations!");
   // We do a +1 here because we want a SourceLocation that means "the end of 
the
   // file", e.g. for the "no newline at the end of the file" diagnostic.
   NextLocalOffset += FileSize + 1;
Index: clang/include/clang/Basic/DiagnosticCommonKinds.td
===
--- clang/include/clang/Basic/DiagnosticCommonKinds.td
+++ clang/include/clang/Basic/DiagnosticCommonKinds.td
@@ -282,6 +282,10 @@
   "file '%0' modified since it was first processed">, DefaultFatal;
 def err_file_too_large : Error<
   "sorry, unsupported: file '%0' is too large for Clang to process">;
+def err_include_too_large : Error<
+  "sorry, this include generates a translation unit too large for"
+  " Clang to process. This may by a result from multiple"
+  " inclusions of unguarded header files.">, DefaultFatal;
 def err_unsupported_bom : Error<"%0 byte order mark detected in '%1', but "
   "encoding is not supported">, DefaultFatal;
 def err_unable_to_rename_temp : Error<


Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -577,13 +577,18 @@
 SLocEntryLoaded[Index] = true;
 return FileID::get(LoadedID);
   }
+  unsigned FileSize = File->getSize();
+  if (!(NextLocalOffset + FileSize + 1 > NextLocalOffset &&
+NextLocalOffset + FileSize + 1 <= CurrentLoadedOffset)) {
+// From this point, there is no sensible way to point to the current
+// source-location and say: This include at line ### generates a too
+// big file, as the IncludePos received is
+Diag.Report(IncludePos, diag::err_include_too_large);
+exit(1);
+  }
   LocalSLocEntryTable.push_back(
   SLocEntry::get(NextLocalOffset,
  FileInfo::get(IncludePos, File, FileCharacter, Filename)));
-  unsigned FileSize = File->getSize();
-  assert(NextLocalOffset + FileSize + 1 > NextLocalOffset &&
- NextLocalOffset + FileSize + 1 <= CurrentLoadedOffset &&
- "Ran out of source locations!");
   // We do a +1 here because we want a SourceLocation that means "the end of the
   // file", e.g. for the "no newline at the end of the file" diagnostic.
   NextLocalOffset += FileSize + 1;
Index: clang/include/clang/Basic/DiagnosticCommonKinds.td
===
--- clang/include/clang/Basic/DiagnosticCommonKinds.td
+++ clang/include/clang/Basic/DiagnosticCommonKinds.td
@@ -282,6 +282,10 @@
   "file '%0' modified since it was first processed">, DefaultFatal;
 def err_file_too_large : Error<
   "sorry, unsupported: file '%0' is too large for Clang to process">;
+def err_include_too_large : Error<
+  "sorry, this include generates a translation unit too large for"
+  " Clang to process. This may by a result from multiple"
+  " inclusions of unguarded header files.">, DefaultFatal;
 def err_unsupported_bom : Error<"%0 byte order mark detected in '%1', but "
   "encoding is not supported">, DefaultFatal;
 def err_unable_to_rename_temp : Error<
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70183: Detect source location overflow due includes

[Mikhail Maltsev via Phabricator via cfe-commits]

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 "

... `>, DefaultFatal;`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70183



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70183: Detect source location overflow due includes

[Mikhail Maltsev via Phabricator via cfe-commits]

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 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70183



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70183: Detect source location overflow due includes

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

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: 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 reach an assert failure 
other than exiting here. Perhaps there is a more llvm specific way to die? 
llvm_unreachable() ?


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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D70183

Files:
  clang/include/clang/Basic/DiagnosticCommonKinds.td
  clang/lib/Basic/SourceManager.cpp


Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -577,13 +577,18 @@
 SLocEntryLoaded[Index] = true;
 return FileID::get(LoadedID);
   }
+  unsigned FileSize = File->getSize();
+  if (!(NextLocalOffset + FileSize + 1 > NextLocalOffset &&
+NextLocalOffset + FileSize + 1 <= CurrentLoadedOffset)) {
+// From this point, there is no sensible way to point to the current
+// source-location and say: This include at line ### generates a too
+// big file, as the IncludePos received is
+Diag.Report(IncludePos, diag::err_include_too_large);
+exit(1);
+  }
   LocalSLocEntryTable.push_back(
   SLocEntry::get(NextLocalOffset,
  FileInfo::get(IncludePos, File, FileCharacter, 
Filename)));
-  unsigned FileSize = File->getSize();
-  assert(NextLocalOffset + FileSize + 1 > NextLocalOffset &&
- NextLocalOffset + FileSize + 1 <= CurrentLoadedOffset &&
- "Ran out of source locations!");
   // We do a +1 here because we want a SourceLocation that means "the end of 
the
   // file", e.g. for the "no newline at the end of the file" diagnostic.
   NextLocalOffset += FileSize + 1;
Index: clang/include/clang/Basic/DiagnosticCommonKinds.td
===
--- clang/include/clang/Basic/DiagnosticCommonKinds.td
+++ clang/include/clang/Basic/DiagnosticCommonKinds.td
@@ -282,6 +282,10 @@
   "file '%0' modified since it was first processed">, DefaultFatal;
 def err_file_too_large : Error<
   "sorry, unsupported: file '%0' is too large for Clang to process">;
+def err_include_too_large : Error<
+  "sorry, this include generates a translation unit too large for"
+  " 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 "
   "encoding is not supported">, DefaultFatal;
 def err_unable_to_rename_temp : Error<


Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -577,13 +577,18 @@
 SLocEntryLoaded[Index] = true;
 return FileID::get(LoadedID);
   }
+  unsigned FileSize = File->getSize();
+  if (!(NextLocalOffset + FileSize + 1 > NextLocalOffset &&
+NextLocalOffset + FileSize + 1 <= CurrentLoadedOffset)) {
+// From this point, there is no sensible way to point to the current
+// source-location and say: This include at line ### generates a too
+// big file, as the IncludePos received is
+Diag.Report(IncludePos, diag::err_include_too_large);
+exit(1);
+  }
   LocalSLocEntryTable.push_back(
   SLocEntry::get(NextLocalOffset,
  FileInfo::get(IncludePos, File, FileCharacter, Filename)));
-  unsigned FileSize = File->getSize();
-  assert(NextLocalOffset + FileSize + 1 > NextLocalOffset &&
- NextLocalOffset + FileSize + 1 <= CurrentLoadedOffset &&
- "Ran out of source locations!");
   // We do a +1 here because we want a SourceLocation that means "the end of the
   // file", e.g. for the "no newline at the end of the file" diagnostic.
   NextLocalOffset += FileSize + 1;
Index: clang/include/clang/Basic/DiagnosticCommonKinds.td
===
--- clang/include/clang/Basic/DiagnosticCommonKinds.td
+++ clang/include/clang/Basic/DiagnosticCommonKinds.td
@@ -282,6 +282,10 @@
   "file '%0' modified since it was first processed">, DefaultFatal;
 def err_file_too_large : Error<
   "sorry, unsupported: file '%0' is too large for Clang to process">;
+def err_include_too_large : Error<
+  "sorry, 

[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);
+  }

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() ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70183



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits