Fix issue 5385: fixit for missing built-in includes

[citisolo]

Summary

When a built-in function was used without including the header a warning and the note 'please include the header' was generated without any fixit information . I have created a patch that adds the fixit advice.

[rsmith]

Thank you for working on this, and sorry it took so long for you to get review feedback.

Most of my comments relate to coding style; please review our style rules at http://llvm.org/docs/CodingStandards.html. You can address many of these comments automatically by using clang-format.

Please only do the work of figuring out the right location for a new #include in the case where we've already decided to emit a diagnostic with a fixit, since it's not especially cheap. Please also factor this out into a separate function, for clarity and because we have other cases where we want to do the same thing (missing module imports, for instance).

lib/Sema/SemaDecl.cpp

1686

In the Clang coding style, comments should start with a capital letter and end in a full stop, and there should be a space after the leading //.

1687–1689

I don't think this will do the right thing if the use is within a macro; you will most likely need to pass Loc through SourceMgr.getFileLoc first.

1688

The * should be on the right, not on the left, in Clang code.

1692

Please insert a space after each comma here.

1694

Indent by 2 spaces, not 4.

1698

You should also check Tok.isAtStartOfLine() here.

1702

If you hit a token that isn't part of a preprocessor directive, you should stop scanning. (A #include half way through a file is probably a .def file or similar, and not a normal include, and in any case, your search will be more efficient.)

1707

You shouldn't use a presumed location here; the FixitLoc should refer to a position within the original source buffer, not into the presumed line numbers space.

1708

Please stay wrap to an 80 column line limit.

1709–1711

No braces around a single-line if body.

[citisolo]

made all the discussed changes to format as well as the below changes;
Lexer.h/.cpp : Added two new methods , findLastDirective and getLineAfterToken
SemaDecl.cpp: modified the code to use new methods
implicit-builtin-decl.c: minor modification to make test pass

[citisolo]

tidied up some leftover comments

[rsmith]

include/clang/Lex/Lexer.h

444

Remove the "return" here: "\returns false if ..."

451–453

Please start comments with a capital letter and end them with a full stop.

lib/Lex/Lexer.cpp

1039

I'm not sure it makes sense to check for a specific directive here. If a file has a mix of #include, #include_next, and #import, we want to find the location after all of them. I think you should make this function less general; specialize it to looking for the "right place to put an include", and just look for the location after the last include-like directive.

Generalizing this to any kind of directive actually makes it less useful, since it prevents us from adding smart heuristics (to deal with #ifs, to try to keep #includes in sorted order, and so on) that only make sense for includes.

1074–1081

This doesn't cover all the cases. If a line ends in a \ (or ??/), or within a /*...*/ comment, we should keep scanning. One way to handle this would be to build a Lexer with whitespace retention enabled, and lex tokens until you find a token at the start of a line.

include/clang/Lex/Lexer.h

     return getCharAndSizeSlowNoWarn(Ptr, Size, LangOpts);
   }
 
+  /// \brief Finds the last  directive in a given file.
+  /// \param FixitLoc stores the result.
+  /// \param Directive specifies the desired directive.
+  /// \return returns false if the directive is not found  and
+  /// \p FixitLoc is defaulted to the start of the file.
+  static bool findLastDirective(const FileID ID, const SourceManager &SM,
+                                const LangOptions &LangOpts,
+                                SourceLocation &LastDirectiveLoc,
+                                llvm::StringRef Directive);
+
+  /// \brief Returns the location of the next line adjacent to the source
+  /// location
+  /// the onus is on the caller to check that the location is valid
+  static SourceLocation getLineAfterToken(const SourceLocation &Loc,
+                                          const SourceManager &SM);
   //===--------------------------------------------------------------------===//
   // Internal implementation interfaces.
 private:

lib/Lex/Lexer.cpp

   return isIdentifierBody(c, LangOpts.DollarIdents);
 }
 
+bool Lexer::findLastDirective(const FileID ID, const SourceManager &SM,
+                              const LangOptions &LangOpts,
+                              SourceLocation &LastDirectiveLoc,
+                              llvm::StringRef Directive) {
+
+  bool FoundDirective = false;
+  // Get the file buffer.
+  bool InvalidTemp = false;
+  llvm::MemoryBuffer *FileBuf = SM.getBuffer(ID, &InvalidTemp);
+  if (InvalidTemp) {
+    LastDirectiveLoc = SourceLocation();
+    return false;
+  }
+
+  // Default location for the Last 'include' directive is start of file.
+  LastDirectiveLoc = SM.getLocForStartOfFile(ID);
+
+  // Find the Location of the last #include directive in the file.
+  Lexer IncludeLexer(ID, FileBuf, SM, LangOpts);
+  while (1) {
+    Token Tok;
+    IncludeLexer.LexFromRawLexer(Tok);
+    if (Tok.is(tok::eof))
+      break;
+    if (Tok.is(tok::hash) && Tok.isAtStartOfLine()) {
+      IncludeLexer.LexFromRawLexer(Tok);
+      if (Tok.getRawIdentifier() == Directive && Tok.getLocation().isValid()) {
+        FoundDirective = true;
+        LastDirectiveLoc = Tok.getLocation();
+      }
+      continue;
+    }
+    // If any other identifier is encountered we've passed the 'includes'.
+    if (tok::isAnyIdentifier(Tok.getKind())) {
+      break;
+    }
+  }
+
+  if (FoundDirective)
+    return true;
+  else
+    return false;
+}
+
+SourceLocation clang::Lexer::getLineAfterToken(const SourceLocation &Loc,
+                                               const SourceManager &SM) {
+
+  SourceLocation FileLoc = SM.getFileLoc(Loc);
+  FileID ID = SM.getFileID(Loc);
+
+  bool InvalidTemp = false;
+  llvm::MemoryBuffer *FileBuf = SM.getBuffer(ID, &InvalidTemp);
+  if (InvalidTemp) {
+    return SourceLocation();
+  }
+
+  const char *BuffEnd = FileBuf->getBufferEnd();
+  const char *RawBuffer = SM.getCharacterData(FileLoc);
+  unsigned char C = *RawBuffer;
+  unsigned OffSet = 0;
+
+  while (1) {
+    if (C == '\n' || C == '\r')
+      break;
+    if (RawBuffer == BuffEnd)
+      return SourceLocation();
+    C = *(++RawBuffer);
+    OffSet++;
+  }
+  OffSet++;
+  return Loc.getLocWithOffset(OffSet);
+}
 
 //===----------------------------------------------------------------------===//
 // Diagnostics forwarding code.

lib/Sema/SemaDecl.cpp

   llvm_unreachable("unhandled error kind");
 }
 
+
 /// LazilyCreateBuiltin - The specified Builtin-ID was first used at
 /// file scope.  lazily create a decl for it. ForRedeclaration is true
 /// if we're creating this built-in in anticipation of redeclaring the

   if (Error) {
     if (ForRedeclaration)
       Diag(Loc, diag::warn_implicit_decl_requires_sysheader)
-          << getHeaderName(Error)
-          << Context.BuiltinInfo.GetName(ID);
+          << getHeaderName(Error) << Context.BuiltinInfo.GetName(ID);
     return nullptr;
   }
 
-  //get the file buffer
-  FileID FileID = SourceMgr.getFileID(Loc);
-  llvm::MemoryBuffer* FileBuf = SourceMgr.getBuffer(FileID);
-  SourceLocation LastIncludeLoc = SourceMgr.getLocForStartOfFile(FileID);
-
-  //Find the Location of the last #include directive in the file
-  Lexer IncludeLexer(FileID,FileBuf,SourceMgr,LangOpts);
-  while (1) {
-      Token Tok;
-      IncludeLexer.LexFromRawLexer(Tok);
-      if (Tok.is(tok::eof))
-          break;
-      if (Tok.is(tok::hash)){
-          IncludeLexer.LexFromRawLexer(Tok);
-          if(Tok.getRawIdentifier() == "include" && Tok.getLocation().isValid())
-                LastIncludeLoc = Tok.getLocation();
-      }
-  }
-
-
-  //Get the location of the line after the last #include directive
-  PresumedLoc PLoc = SourceMgr.getPresumedLoc(LastIncludeLoc);
-  SourceLocation FixitLoc = SourceMgr.translateLineCol(FileID,PLoc.getLine()+1,PLoc.getColumn());
-  if (FixitLoc.isInvalid()){
-      FixitLoc = LastIncludeLoc;
-  }
-
+  SourceLocation FileLoc = SourceMgr.getFileLoc(Loc);
+  FileID FixitFileID = SourceMgr.getFileID(FileLoc);
 
+  SourceLocation FixitLoc;
+  if (Lexer::findLastDirective(FixitFileID, SourceMgr, LangOpts, FixitLoc,
+                               "include"))
+    FixitLoc = Lexer::getLineAfterToken(FixitLoc, SourceMgr);
 
   if (!ForRedeclaration && Context.BuiltinInfo.isPredefinedLibFunction(ID)) {
     Diag(Loc, diag::ext_implicit_lib_function_decl)
       << Context.BuiltinInfo.GetName(ID)
       << R;
-    //Build 'insert missing header' string
+    //Build 'insert missing header' string.
     std::string FixitString = "#include <";
     FixitString.append(Context.BuiltinInfo.getHeaderName(ID));
     FixitString.append(">");

test/Sema/implicit-builtin-decl.c

 
 void f() {
   int *ptr = malloc(sizeof(int) * 10); // expected-warning{{implicitly declaring library function 'malloc' with type}} \
-  // expected-note@-3{{include the header <stdlib.h> or explicitly provide a declaration for 'malloc'}} \
+  // expected-note@-4{{include the header <stdlib.h> or explicitly provide a declaration for 'malloc'}} \
   // expected-note{{'malloc' is a builtin with type 'void *}}
 }