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

[hokein]

Summary

The crash is casued by an assertion in StringRef. (llvm::StringRef::front() const: Assertion `!empty()' failed)

[sammccall]

lib/Lex/PPDirectives.cpp

1891

everything in this block is guarded by !Filename.empty().
Just add it to the if condition?

1926

this is mysterious - what does it solve? is it the right place to handle this problem?

(You allude to a code complete crash - can you explain?)

[kristina]

Able to reproduce easily:

clang-8: /SourceCache/llvm-trunk-8.0/include/llvm/ADT/StringRef.h:143: char llvm::StringRef::front() const: Assertion `!empty()' failed.

Will try it with patches applied in a moment.

[kristina]

lib/Lex/PPDirectives.cpp

1891–1892

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.

[kristina]

lib/Lex/PPDirectives.cpp

1891–1892

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.

[hokein]

Update the code.

[hokein]

lib/Lex/PPDirectives.cpp

1891

The logic is tricky... inside the if body, we modify Filename, so we can't just add the judgement here.

1891–1892

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

1926

ah, I created this patch when fixing a clangd crash, it seems still crash clangd without it, but we can pass the test without it. I think that is another issue, so I remove this code now, and address it in a follow-up patch.

[sammccall]

lib/Lex/PPDirectives.cpp

1910

Filename = Filename.drop_until(isAlphanumeric)

(unfortunately there's no equivalent "back" version)

1913

simplify the logic by merging with the while loop? (and drop the assert)

1924

if this early bail-out isn't necessary, please drop it - it's optimizing an extremely uncommon case

[kristina]

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.

lib/Lex/PPDirectives.cpp

1891–1892

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.

[kristina]

lib/Lex/PPDirectives.cpp

1913

I thought the assert was a good idea in case a similar issue popped up again (somehow triggering this but in reverse, not sure if that makes sense), it's a no-op in upstream release builds anyway.

[sammccall]

lib/Lex/PPDirectives.cpp

1913

while (!empty && !isAlpha(back)) { drop_back }
is just as safe, and easier to understand than

// ... subtly establish that there is an alphanum in the string ...
while (!isAlpha(back)) { drop_back; assert(!empty) }

[kristina]

Yes I think the clangd crash will have to go in another diff, but this fixes the crash in clang, which is pretty good in itself. I don't build clangd at the moment and have no buildbot available so I can try to look into it later if you can open a Bugzilla thing with steps to reproduce maybe unless Sam wants to dig into it?

[kristina]

Please make sure it builds after you update the revision, I usually do it myself but it'll take too long on my desktop and I accidentally broke the hypervisor on my buildbot so can't do it this time.

lib/Lex/PPDirectives.cpp

1913

Fair enough, that would be better. Don't think I have anything else to add, I think this revision just requires addressing that and then it's good to go, but see my comment on clangd.

[hokein]

Address review comments.

[hokein]

I verified this patch is passed all clang tests (ninja check-clang).

lib/Lex/PPDirectives.cpp

if (!SuppressIncludeNotFoundError) {
             Filename <<
             FixItHint::CreateReplacement(Range, "\"" + Filename.str() + "\"");
         }
       }
 
       // Check for likely typos due to leading or trailing non-isAlphanumeric
       // characters
       StringRef OriginalFilename = Filename;
       if (!File) {
-        while (!isAlphanumeric(Filename.front())) {
-          Filename = Filename.drop_front();
-        }
-        while (!isAlphanumeric(Filename.back())) {
+        // A heuristic to correct a typo file name by removing leading and
+        // trailing non-isAlphanumeric characters.
+        auto CorrectTypoFilename = [](llvm::StringRef Filename) {
+          Filename = Filename.drop_until(isAlphanumeric);
+          while (!Filename.empty() && !isAlphanumeric(Filename.back())) {
             Filename = Filename.drop_back();
           }
-
+          return Filename;
+        };
+        Filename = CorrectTypoFilename(Filename);
         File = LookupFile(
             FilenameLoc,
             LangOpts.MSVCCompat ? NormalizedPath.c_str() : Filename, isAngled,
             LookupFrom, LookupFromFile, CurDir,
             Callbacks ? &SearchPath : nullptr,
             Callbacks ? &RelativePath : nullptr, &SuggestedModule, &IsMapped);
         if (File) {
           SourceRange Range(FilenameTok.getLocation(), CharEnd);
           auto Hint = isAngled ? FixItHint::CreateReplacement(
                                      Range, "<" + Filename.str() + ">")
                                : FixItHint::CreateReplacement(
                                      Range, "\"" + Filename.str() + "\"");
           Diag(FilenameTok, diag::err_pp_file_not_found_typo_not_fatal)
               << OriginalFilename << Filename << Hint;
         }
       }
 
       // If the file is still not found, just go with the vanilla diagnostic
       if (!File)
         Diag(FilenameTok, diag::err_pp_file_not_found) << OriginalFilename
                                                        << FilenameRange;
     }
   }
 
   if (usingPCHWithThroughHeader() && SkippingUntilPCHThroughHeader) {
     if (isPCHThroughHeader(File))
       SkippingUntilPCHThroughHeader = false;
     return;
   }
 
   // Should we enter the source file? Set to false if either the source file is
   // known to have no effect beyond its effect on module visibility -- that is,
   // if it's got an include guard that is already defined or is a modular header

test/Preprocessor/include-nonalpha-no-crash.c

+// RUN: %clang_cc1 %s -verify
+
+#include "./" // expected-error {{'./' file not found}}