Use filename in linemarker when compiling preprocessed source

[twoh]

Summary

Clang appears to always use name as specified on the command
line, whereas gcc uses the name as specified in the linemarker at the
first line when compiling a preprocessed source. This results mismatch
between two compilers in FILE symbol table entry. This patch makes clang
to resemble gcc's behavior in finding the original source file name and
use it as an input file name.

Even with this patch, values of FILE symbol table entry may still be
different because clang uses dirname+basename for the entry whlie gcc
uses basename only. I'll write a patch for that once this patch is
committed.

[inglorion]

Hey @twoh! Can you say a little more about the larger picture here? What part of our output is different from gcc's and what does that affect? We already have code that processes linemarkers (e.g. https://reviews.llvm.org/diffusion/L/browse/cfe/trunk/lib/Lex/PPDirectives.cpp;292239$1296) - can you use that to get the right output?

lib/Frontend/FrontendAction.cpp

390 ↗

(On Diff #84637)

This seems fragile. Does the directive specifying the source file name only apply to preprocessed files, or can we remove the if statement here and always look for the marker? If we must have a special case for preprocessed files, can we add a method to FrontendInputFile to check for preprocessed files? Also see my other comment about the existing processing of linemarkers, which also processes markers not at the beginning of the file.

[twoh]

Hello @inglorion, Thanks for the comment! Following is the repro for the issue we have:

$ touch test.c
$ gcc -E -o test.i test.c
$ gcc -c -o test.o test.i
$ readelf -a test.o | grep test
1: 0000000000000000 0 FILE LOCAL DEFAULT ABS test.c
$ clang test.c -E -o test.i
$ clang -c -o test.o test.i
$ readelf -a test.o | grep test
1: 0000000000000000 0 FILE LOCAL DEFAULT ABS test.i
$ clang -c -o test.o test.c
$ readelf -a test.o | grep test
1: 0000000000000000 0 FILE LOCAL DEFAULT ABS test.c

As you can see, FILE symbol is different for gcc and clang if preprocessed file is given as an input.

I'm trying to resemble the gcc behavior of https://github.com/gcc-mirror/gcc/blob/master/libcpp/init.c#L641-L647. It is only for preprocessed file, and only for the first line. I was thinking of using existing code for handling linemarkers as well, but decided not to do it because it may slowdown the general cases.

I like your idea of having a method to check if it is preprocessed input. I'll update it shortly. Thanks!

[twoh]

Addressing comments from @inglorion

[inglorion]

I added a couple of comments about the test. I still want to take a look at if we can get the file name using the existing code. It may take me a few days before I can get to that, so I'm sending the comments on the test in the meantime.

test/Frontend/preprocessed-input.c

2 ↗

(On Diff #84711)

We don't put the input filename in the object file for all targets. To make the test portable, you can add "-triple i686-linux-gnu" to the arguments you're passing to clang.

3 ↗

(On Diff #84711)

I don't think readelf is necessarily available on all systems. Can you use llvm-objdump -t instead?

[twoh]

Addressing comments about the test from @inglorion. Thanks!

[twoh]

Something was wrong with the previous update.

[inglorion]

I've had some more time to look at this, and I think your approach is basically good. My idea of reading the source location after we've done some parsing would be a lot more work to implement: the string we care about is currently passed as a StringRef to the functions BeginSourceFile calls; to implement what I was thinking, we would need some object we could query for the name. Let's punt on that. With that said, I have a couple of inline comments. Once you've addressed those, I'm happy with the diff.

lib/Frontend/FrontendAction.cpp

192 ↗

(On Diff #84889)

Since you're relying on InputFile not being null, please make it a reference instead of a pointer.

204 ↗

(On Diff #84889)

Two comments here. First, the code reads tokens until it reaches the next line, but we really only care about the first 3 tokens, and only if they are hash, numeric literal, and string literal, in that order. Secondly, there is a handy class StringLiteralParser which will decode strings in source code, which is probably more correct than just stripping quotes (and also what the other linemarker processing code uses). How about:

This will read tokens until we reach the next line, but we really only care about the first three, and only if the first two are a hash and a numeric literal. How about:

Token T;
if (RawLexer->LexFromRawLexer(T) || T.getKind() != tok::hash)
  return false;
if (RawLexer->LexFromRawLexer(T) || T.isAtStartOfLine() ||
    T.getKind() != tok::numeric_constant)
  return false;
if (RawLexer->LexFromRawLexer(T) || T.isAtStartOfLine() ||
    T.getKind() != tok::string_literal)
  return false;

StringLiteralParser Literal(T, CI.getPreprocessor());
InputFile = Literal.GetString().str();
return true;

You will need to #include "clang/Lex/LiteralSupport.h"

389 ↗

(On Diff #84889)

The lifetime of this string doesn't match the lifetime of the string that normally backs the filename (which is stored in Input). I don't think anything currently depends on the StringRef being valid after BeginSourceFile returns, but it may be worth adding a comment to that effect (or extending the lifetime).

[twoh]

lib/Frontend/FrontendAction.cpp

192 ↗

(On Diff #84889)

I made it a pointer to make it clear that InputFile can be modified inside this function. Does LLVM have a guideline for this? I couldn't find one from http://llvm.org/docs/CodingStandards.html.

[twoh]

Addressing comments from @inglorion. Thanks!

[inglorion]

lgtm!

[twoh]

Thanks!

[twoh]

Hello @inglorion, I'm experiencing an issue with landing this patch with my userid now. If you have a commit privilege, can you please land it for me?

cfe/trunk/include/clang/Frontend/FrontendOptions.h

enum InputKind {
   IK_OpenCL,
   IK_CUDA,
   IK_PreprocessedCuda,
   IK_RenderScript,
   IK_AST,
   IK_LLVM_IR
 };
 
-  
 /// \brief An input file for the front end.
 class FrontendInputFile {
   /// \brief The file name, or "-" to read from standard input.
   std::string File;
 
   llvm::MemoryBuffer *Buffer;
 
   /// \brief The kind of input, e.g., C source, AST file, LLVM IR.
FrontendInputFile(llvm::MemoryBuffer *buffer, InputKind Kind,
     : Buffer(buffer), Kind(Kind), IsSystem(IsSystem) { }
 
   InputKind getKind() const { return Kind; }
   bool isSystem() const { return IsSystem; }
 
   bool isEmpty() const { return File.empty() && Buffer == nullptr; }
   bool isFile() const { return !isBuffer(); }
   bool isBuffer() const { return Buffer != nullptr; }
+  bool isPreprocessed() const {
+    return Kind == IK_PreprocessedC ||
+           Kind == IK_PreprocessedCXX ||
+           Kind == IK_PreprocessedObjC ||
+           Kind == IK_PreprocessedObjCXX ||
+           Kind == IK_PreprocessedCuda;
+  }
 
   StringRef getFile() const {
     assert(isFile());
     return File;
   }
   llvm::MemoryBuffer *getBuffer() const {
     assert(isBuffer());
     return Buffer;

cfe/trunk/lib/Frontend/FrontendAction.cpp

 #include "clang/Frontend/ASTUnit.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/FrontendDiagnostic.h"
 #include "clang/Frontend/FrontendPluginRegistry.h"
 #include "clang/Frontend/LayoutOverrideSource.h"
 #include "clang/Frontend/MultiplexConsumer.h"
 #include "clang/Frontend/Utils.h"
 #include "clang/Lex/HeaderSearch.h"
+#include "clang/Lex/LiteralSupport.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "clang/Parse/ParseAST.h"
 #include "clang/Serialization/ASTDeserializationListener.h"
 #include "clang/Serialization/ASTReader.h"
 #include "clang/Serialization/GlobalModuleIndex.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FileSystem.h"
FrontendAction::CreateWrappedASTConsumer(CompilerInstance &CI,
   Consumers.push_back(std::move(Consumer));
   for (auto &C : AfterConsumers) {
     Consumers.push_back(std::move(C));
   }
 
   return llvm::make_unique<MultiplexConsumer>(std::move(Consumers));
 }
 
+// For preprocessed files, if the first line is the linemarker and specifies
+// the original source file name, use that name as the input file name.
+static bool ReadOriginalFileName(CompilerInstance &CI, std::string &InputFile)
+{
+  bool Invalid = false;
+  auto &SourceMgr = CI.getSourceManager();
+  auto MainFileID = SourceMgr.getMainFileID();
+  const auto *MainFileBuf = SourceMgr.getBuffer(MainFileID, &Invalid);
+  if (Invalid)
+    return false;
+
+  std::unique_ptr<Lexer> RawLexer(
+      new Lexer(MainFileID, MainFileBuf, SourceMgr, CI.getLangOpts()));
+
+  // If the first line has the syntax of
+  //
+  // # NUM "FILENAME"
+  //
+  // we use FILENAME as the input file name.
+  Token T;
+  if (RawLexer->LexFromRawLexer(T) || T.getKind() != tok::hash)
+    return false;
+  if (RawLexer->LexFromRawLexer(T) || T.isAtStartOfLine() ||
+      T.getKind() != tok::numeric_constant)
+    return false;
+  RawLexer->LexFromRawLexer(T);
+  if (T.isAtStartOfLine() || T.getKind() != tok::string_literal)
+    return false;
+
+  StringLiteralParser Literal(T, CI.getPreprocessor());
+  if (Literal.hadError)
+    return false;
+  InputFile = Literal.GetString().str();
+  return true;
+}
+
 bool FrontendAction::BeginSourceFile(CompilerInstance &CI,
                                      const FrontendInputFile &Input) {
   assert(!Instance && "Already processing a source file!");
   assert(!Input.isEmpty() && "Unexpected empty filename!");
   setCurrentInput(Input);
   setCompilerInstance(&CI);
 
   StringRef InputFile = Input.getFile();
bool FrontendAction::BeginSourceFile(CompilerInstance &CI,
 
   // Create the AST context and consumer unless this is a preprocessor only
   // action.
   if (!usesPreprocessorOnly()) {
     // Parsing a model file should reuse the existing ASTContext.
     if (!isModelParsingAction())
       CI.createASTContext();
 
+    // For preprocessed files, check if the first line specifies the original
+    // source file name with a linemarker.
+    std::string OrigFile;
+    if (Input.isPreprocessed())
+      if (ReadOriginalFileName(CI, OrigFile))
+        InputFile = OrigFile;
+
     std::unique_ptr<ASTConsumer> Consumer =
         CreateWrappedASTConsumer(CI, InputFile);
     if (!Consumer)
       goto failure;
 
     // FIXME: should not overwrite ASTMutationListener when parsing model files?
     if (!isModelParsingAction())
       CI.getASTContext().setASTMutationListener(Consumer->GetASTMutationListener());
bool FrontendAction::BeginSourceFile(CompilerInstance &CI,
 
   // If we were asked to load any module files, do so now.
   for (const auto &ModuleFile : CI.getFrontendOpts().ModuleFiles)
     if (!CI.loadModuleFile(ModuleFile))
       goto failure;
 
   // If there is a layout overrides file, attach an external AST source that
   // provides the layouts from that file.
-  if (!CI.getFrontendOpts().OverrideRecordLayoutsFile.empty() && 
+  if (!CI.getFrontendOpts().OverrideRecordLayoutsFile.empty() &&
       CI.hasASTContext() && !CI.getASTContext().getExternalSource()) {
-    IntrusiveRefCntPtr<ExternalASTSource> 
+    IntrusiveRefCntPtr<ExternalASTSource>
       Override(new LayoutOverrideSource(
                      CI.getFrontendOpts().OverrideRecordLayoutsFile));
     CI.getASTContext().setExternalSource(Override);
   }
 
   return true;
 
   // If we failed, reset state since the client will not end up calling the

cfe/trunk/test/Frontend/preprocessed-input.c

+// RUN: %clang -E -o %t.i %s
+// RUN: %clang -Xclang -triple -Xclang i686-linux-gnu -c -o %t.o %t.i
+// RUN: llvm-objdump -t %t.o | FileCheck %s
+// CHECK: l{{ +}}df{{ +}}*ABS*{{ +}}{{0+}}{{.+}}preprocessed-input.c{{$}}