[PATCH] D52721: [Preprocessor] Fix a crash when handling non-alpha include header.

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

Have you tested that build is successful and obviously the FileCheck test should pass now? What sort of behavior does this produce when you attempt to use an invalid path like that now when compiling with clang? Just a predecessor diagnostic?

I'm updating software on my buildbot so I can't really do a quick run to test it right now, though if you definitely know it compiles and the test passes, I'm happy to approve this.



================
Comment at: lib/Lex/PPDirectives.cpp:1892
       if (!File) {
-        while (!isAlphanumeric(Filename.front())) {
+        while (!Filename.empty() && !isAlphanumeric(Filename.front())) {
           Filename = Filename.drop_front();
----------------
hokein wrote:
> kristina wrote:
> > kristina wrote:
> > > This line is tripping the assert, it seems best course of action would be a single check here and then just diagnosing an error unless you have managed to find other cases, in which case all the checks below are also warranted.
> > In either case, the diagnostic emitted doesn't really make sense, at least to me, I think it would be better to explicitly diagnose this case as an error and then bail, before an assertion fires.
> > 
> > I also crashed clang with `#include "./"`, so the test case does seem to be fairly minimal which is good. Though I think a diagnostic about a bogus file path would be better (I don't know how to word it well), rather than saying file not found.
> Thanks for the comment.
> 
> IIUC, do you mean we create a new diagnostic message for this specific case? I'm double that it is worth doing it, since IMO this is a heuristic that getting a typo file path, and user input is arbitrary, make the diagnostic fit all users input would be very tricky.
> 
> I think we can just provide best-effort for this case (e.g. no crash), "err_pp_file_not_found" seems the best one from existing messages. 
> 
> I checked with `g++,` it provides a slightly better message ("fatal error: ./: No such file or directory" vs clang's "'./' file not found").
> 
Hm, I think it's up to you if you want to make a new diagnostic or reuse one that's similar enough, as long as you emit it and then bail out before you hit the assertion, that should fix the bug. I like the new asserts as well.


Repository:
  rC Clang

https://reviews.llvm.org/D52721