Re: [PATCH] D16135: Macro Debug Info support in Clang

[David Blaikie via cfe-commits]

r294676

On Thu, Feb 9, 2017 at 4:05 PM David L. Jones via Phabricator <
revi...@reviews.llvm.org> wrote:

> dlj added inline comments.
>
>
> 
> Comment at: cfe/trunk/lib/CodeGen/MacroPPCallbacks.cpp:125
> +  switch (Status) {
> +  default:
> +llvm_unreachable("Do not expect to enter a file from current scope");
> 
> As a heads up... this fails under -Werror:
>
> llvm/tools/clang/lib/CodeGen/MacroPPCallbacks.cpp:125:3: error: default
> label in switch which covers all enumeration values
> [-Werror,-Wcovered-switch-default]
>   default:
>   ^
> 1 error generated.
>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D16135
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D16135: Macro Debug Info support in Clang

[David L. Jones via Phabricator via cfe-commits]

dlj added inline comments.



Comment at: cfe/trunk/lib/CodeGen/MacroPPCallbacks.cpp:125
+  switch (Status) {
+  default:
+llvm_unreachable("Do not expect to enter a file from current scope");

As a heads up... this fails under -Werror:

llvm/tools/clang/lib/CodeGen/MacroPPCallbacks.cpp:125:3: error: default label 
in switch which covers all enumeration values [-Werror,-Wcovered-switch-default]
  default:
  ^
1 error generated.


Repository:
  rL LLVM

https://reviews.llvm.org/D16135



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

[PATCH] D16135: Macro Debug Info support in Clang

[Amjad Aboud via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL294637: [DebugInfo] Added support to Clang FE for generating 
debug info for… (authored by aaboud).

Changed prior to commit:
  https://reviews.llvm.org/D16135?vs=87819=87881#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D16135

Files:
  cfe/trunk/docs/UsersManual.rst
  cfe/trunk/include/clang/CodeGen/ModuleBuilder.h
  cfe/trunk/include/clang/Driver/CC1Options.td
  cfe/trunk/include/clang/Driver/Options.td
  cfe/trunk/include/clang/Frontend/CodeGenOptions.def
  cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
  cfe/trunk/lib/CodeGen/CGDebugInfo.h
  cfe/trunk/lib/CodeGen/CMakeLists.txt
  cfe/trunk/lib/CodeGen/CodeGenAction.cpp
  cfe/trunk/lib/CodeGen/MacroPPCallbacks.cpp
  cfe/trunk/lib/CodeGen/MacroPPCallbacks.h
  cfe/trunk/lib/CodeGen/ModuleBuilder.cpp
  cfe/trunk/lib/Driver/Tools.cpp
  cfe/trunk/lib/Frontend/CompilerInvocation.cpp
  cfe/trunk/test/CodeGen/debug-info-macro.c
  cfe/trunk/test/CodeGen/include/debug-info-macro.h
  cfe/trunk/test/Driver/debug-options.c

Index: cfe/trunk/lib/CodeGen/CGDebugInfo.h
===
--- cfe/trunk/lib/CodeGen/CGDebugInfo.h
+++ cfe/trunk/lib/CodeGen/CGDebugInfo.h
@@ -413,6 +413,16 @@
 
   void completeTemplateDefinition(const ClassTemplateSpecializationDecl );
 
+  /// Create debug info for a macro defined by a #define directive or a macro
+  /// undefined by a #undef directive.
+  llvm::DIMacro *CreateMacro(llvm::DIMacroFile *Parent, unsigned MType,
+ SourceLocation LineLoc, StringRef Name,
+ StringRef Value);
+
+  /// Create debug info for a file referenced by an #include directive.
+  llvm::DIMacroFile *CreateTempMacroFile(llvm::DIMacroFile *Parent,
+ SourceLocation LineLoc,
+ SourceLocation FileLoc);
 private:
   /// Emit call to llvm.dbg.declare for a variable declaration.
   void EmitDeclare(const VarDecl *decl, llvm::Value *AI,
Index: cfe/trunk/lib/CodeGen/CodeGenAction.cpp
===
--- cfe/trunk/lib/CodeGen/CodeGenAction.cpp
+++ cfe/trunk/lib/CodeGen/CodeGenAction.cpp
@@ -10,6 +10,7 @@
 #include "clang/CodeGen/CodeGenAction.h"
 #include "CodeGenModule.h"
 #include "CoverageMappingGen.h"
+#include "MacroPPCallbacks.h"
 #include "clang/AST/ASTConsumer.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/DeclCXX.h"
@@ -97,6 +98,8 @@
   return std::unique_ptr(Gen->ReleaseModule());
 }
 
+CodeGenerator *getCodeGenerator() { return Gen.get(); }
+
 void HandleCXXStaticMemberVarInstantiation(VarDecl *VD) override {
   Gen->HandleCXXStaticMemberVarInstantiation(VD);
 }
@@ -830,6 +833,17 @@
   CI.getLangOpts(), CI.getFrontendOpts().ShowTimers, InFile,
   std::move(LinkModules), std::move(OS), *VMContext, CoverageInfo));
   BEConsumer = Result.get();
+
+  // Enable generating macro debug info only when debug info is not disabled and
+  // also macro debug info is enabled.
+  if (CI.getCodeGenOpts().getDebugInfo() != codegenoptions::NoDebugInfo &&
+  CI.getCodeGenOpts().MacroDebugInfo) {
+std::unique_ptr Callbacks =
+llvm::make_unique(BEConsumer->getCodeGenerator(),
+CI.getPreprocessor());
+CI.getPreprocessor().addPPCallbacks(std::move(Callbacks));
+  }
+
   return std::move(Result);
 }
 
Index: cfe/trunk/lib/CodeGen/MacroPPCallbacks.cpp
===
--- cfe/trunk/lib/CodeGen/MacroPPCallbacks.cpp
+++ cfe/trunk/lib/CodeGen/MacroPPCallbacks.cpp
@@ -0,0 +1,211 @@
+//===--- MacroPPCallbacks.cpp -===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+//  This file contains implementation for the macro preprocessors callbacks.
+//
+//===--===//
+
+#include "MacroPPCallbacks.h"
+#include "CGDebugInfo.h"
+#include "clang/CodeGen/ModuleBuilder.h"
+#include "clang/Parse/Parser.h"
+
+using namespace clang;
+
+void MacroPPCallbacks::writeMacroDefinition(const IdentifierInfo ,
+const MacroInfo ,
+Preprocessor , raw_ostream ,
+raw_ostream ) {
+  Name << II.getName();
+
+  if (MI.isFunctionLike()) {
+Name << '(';
+if (!MI.arg_empty()) {
+  MacroInfo::arg_iterator AI = MI.arg_begin(), E = MI.arg_end();
+  for (; AI + 1 != E; ++AI) {
+Name << (*AI)->getName();
+Name << ',';
+  }
+
+  // Last argument.
+   

[PATCH] D16135: Macro Debug Info support in Clang

[Adrian Prantl via Phabricator via cfe-commits]

aprantl accepted this revision.
aprantl added a comment.
This revision is now accepted and ready to land.

Could you also add support for -g3? It looks like we are supporting -g0 and 
-g1, so we should probably also mirror -g3.

Thanks a lot for all your work!


https://reviews.llvm.org/D16135



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

[PATCH] D16135: Macro Debug Info support in Clang

[Amjad Aboud via Phabricator via cfe-commits]

aaboud updated this revision to Diff 87819.
aaboud marked 2 inline comments as done.
aaboud added a comment.

Added test cases for driver debug info macro flags in 
test/Driver/debug-options.c
Applied some changes to the documentation suggested by Ardian.


https://reviews.llvm.org/D16135

Files:
  docs/UsersManual.rst
  include/clang/CodeGen/ModuleBuilder.h
  include/clang/Driver/CC1Options.td
  include/clang/Driver/Options.td
  include/clang/Frontend/CodeGenOptions.def
  lib/CodeGen/CGDebugInfo.cpp
  lib/CodeGen/CGDebugInfo.h
  lib/CodeGen/CMakeLists.txt
  lib/CodeGen/CodeGenAction.cpp
  lib/CodeGen/MacroPPCallbacks.cpp
  lib/CodeGen/MacroPPCallbacks.h
  lib/CodeGen/ModuleBuilder.cpp
  lib/Driver/Tools.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/debug-info-macro.c
  test/CodeGen/include/debug-info-macro.h
  test/Driver/debug-options.c

Index: lib/CodeGen/ModuleBuilder.cpp
===
--- lib/CodeGen/ModuleBuilder.cpp
+++ lib/CodeGen/ModuleBuilder.cpp
@@ -92,6 +92,10 @@
   return M.get();
 }
 
+CGDebugInfo *getCGDebugInfo() {
+  return Builder->getModuleDebugInfo();
+}
+
 llvm::Module *ReleaseModule() {
   return M.release();
 }
@@ -299,6 +303,10 @@
   return static_cast(this)->ReleaseModule();
 }
 
+CGDebugInfo *CodeGenerator::getCGDebugInfo() {
+  return static_cast(this)->getCGDebugInfo();
+}
+
 const Decl *CodeGenerator::GetDeclForMangledName(llvm::StringRef name) {
   return static_cast(this)->GetDeclForMangledName(name);
 }
Index: lib/CodeGen/MacroPPCallbacks.h
===
--- lib/CodeGen/MacroPPCallbacks.h
+++ lib/CodeGen/MacroPPCallbacks.h
@@ -0,0 +1,118 @@
+//===--- MacroPPCallbacks.h -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+//  This file defines implementation for the macro preprocessors callbacks.
+//
+//===--===//
+
+#include "clang/Lex/PPCallbacks.h"
+
+namespace llvm {
+class DIMacroFile;
+class DIMacroNode;
+}
+namespace clang {
+class Preprocessor;
+class MacroInfo;
+class CodeGenerator;
+
+class MacroPPCallbacks : public PPCallbacks {
+  /// A pointer to code generator, where debug info generator can be found.
+  CodeGenerator *Gen;
+
+  /// Preprocessor.
+  Preprocessor 
+
+  /// Location of recent included file, used for line number.
+  SourceLocation LastHashLoc;
+
+  /// Counts current number of command line included files, which were entered
+  /// and were not exited yet.
+  int EnteredCommandLineIncludeFiles = 0;
+
+  enum FileScopeStatus {
+NoScope = 0,  // Scope is not initialized yet.
+InitializedScope, // Main file scope is initialized but not set yet.
+BuiltinScope, //  and  file scopes.
+CommandLineIncludeScope,  // Included file, from  file, scope.
+MainFileScope // Main file scope.
+  };
+  FileScopeStatus Status;
+
+  /// Parent contains all entered files that were not exited yet according to
+  /// the inclusion order.
+  llvm::SmallVector Scopes;
+
+  /// Get current DIMacroFile scope.
+  /// \return current DIMacroFile scope or nullptr if there is no such scope.
+  llvm::DIMacroFile *getCurrentScope();
+
+  /// Get current line location or invalid location.
+  /// \param Loc current line location.
+  /// \return current line location \p `Loc`, or invalid location if it's in a
+  /// skipped file scope.
+  SourceLocation getCorrectLocation(SourceLocation Loc);
+
+  /// Use the passed preprocessor to write the macro name and value from the
+  /// given macro info and identifier info into the given \p `Name` and \p
+  /// `Value` output streams.
+  ///
+  /// \param II Identifier info, used to get the Macro name.
+  /// \param MI Macro info, used to get the Macro argumets and values.
+  /// \param PP Preprocessor.
+  /// \param [out] Name Place holder for returned macro name and arguments.
+  /// \param [out] Value Place holder for returned macro value.
+  static void writeMacroDefinition(const IdentifierInfo ,
+   const MacroInfo , Preprocessor ,
+   raw_ostream , raw_ostream );
+
+  /// Update current file scope status to next file scope.
+  void updateStatusToNextScope();
+
+  /// Handle the case when entering a file.
+  ///
+  /// \param Loc Indicates the new location.
+  /// \Return true if file scope status should be updated.
+  void FileEntered(SourceLocation Loc);
+
+  /// Handle the case when exiting a file.
+  ///
+  /// \Return true if file scope status should be 

[PATCH] D16135: Macro Debug Info support in Clang

[Adrian Prantl via Phabricator via cfe-commits]

aprantl added a comment.

Please also add driver testcase (e.g., to test/Driver/debug-options.c).
Technically clang does accept -g3 as an option, so we could wire it up for 
compatibility. I would still keep the -fmacro-debug driver option though.




Comment at: docs/UsersManual.rst:1692
+
+Debug info for macro increases the size of debug information in the binary.
+Macro debug info generated by Clang can be controlled by the flags listed 
below.

Debug info for C preprocessor macros ...



Comment at: docs/UsersManual.rst:1697
+
+  Generate complete debug info. This flag is discarded when **-g0** is enabled.
+

Generate debug info for preprocessor macros. ...


https://reviews.llvm.org/D16135



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

[PATCH] D16135: Macro Debug Info support in Clang

[Amjad Aboud via Phabricator via cfe-commits]

aaboud added a comment.

> How are you measuring the build time? Total time for, say "ninja clang" with 
> full parallelism? That'd be hard to measure the actual impact (since it could 
> be the link time or other things are dominating, etc). If you have a reliable 
> way to time (I'm assuming Intel has lots of tools for measuring compiler 
> performance) the compilation, get a sense of the variance, etc (& link time, 
> to get a sense of how much the larger inputs affect linker performance) would 
> be useful.

I did a manual measurement, but I believe it is representative.
I used "time" tool to measure the whole time of the build (compile + link).

- time make -j8

Also, notice that I limited the targets to X86 only, so it was not a full build 
of LLVM.

- -DLLVM_OPTIMIZED_TABLEGEN=ON -DLLVM_TARGETS_TO_BUILD="X86" 
-DCMAKE_CXX_FLAGS="-fstandalone-debug" -DCMAKE_C_FLAGS="-fstandalone-debug"

However, as I already said, it should answer the time change and size question 
fairly enough.

> Does GCC have a command line option for this that we could mirror?

GCC emits macro debug info when when build with -g3.


https://reviews.llvm.org/D16135



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

[PATCH] D16135: Macro Debug Info support in Clang

[Amjad Aboud via Phabricator via cfe-commits]

aaboud updated this revision to Diff 87796.
aaboud added a comment.

Added flag to control macro debug info generation:

1. -fdebug-macro (-fno-debug-macro) for Clang driver
2. -debug-info-macro for Clang compiler (-cc1)

Also, made sure FE will discard this flag when debug info is not required, i.e. 
with -g0.
Should we discard the macro debug info when "-gline-tables-only" is used? or it 
is up to the user not to use "-fdebug-macro" in this case?

I also extended the test to check all combinations of using -debug-info-macro 
flag with -debug-info-kind flag.


https://reviews.llvm.org/D16135

Files:
  docs/UsersManual.rst
  include/clang/CodeGen/ModuleBuilder.h
  include/clang/Driver/CC1Options.td
  include/clang/Driver/Options.td
  include/clang/Frontend/CodeGenOptions.def
  lib/CodeGen/CGDebugInfo.cpp
  lib/CodeGen/CGDebugInfo.h
  lib/CodeGen/CMakeLists.txt
  lib/CodeGen/CodeGenAction.cpp
  lib/CodeGen/MacroPPCallbacks.cpp
  lib/CodeGen/MacroPPCallbacks.h
  lib/CodeGen/ModuleBuilder.cpp
  lib/Driver/Tools.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/debug-info-macro.c
  test/CodeGen/include/debug-info-macro.h

Index: lib/CodeGen/ModuleBuilder.cpp
===
--- lib/CodeGen/ModuleBuilder.cpp
+++ lib/CodeGen/ModuleBuilder.cpp
@@ -92,6 +92,10 @@
   return M.get();
 }
 
+CGDebugInfo *getCGDebugInfo() {
+  return Builder->getModuleDebugInfo();
+}
+
 llvm::Module *ReleaseModule() {
   return M.release();
 }
@@ -299,6 +303,10 @@
   return static_cast(this)->ReleaseModule();
 }
 
+CGDebugInfo *CodeGenerator::getCGDebugInfo() {
+  return static_cast(this)->getCGDebugInfo();
+}
+
 const Decl *CodeGenerator::GetDeclForMangledName(llvm::StringRef name) {
   return static_cast(this)->GetDeclForMangledName(name);
 }
Index: lib/CodeGen/MacroPPCallbacks.h
===
--- lib/CodeGen/MacroPPCallbacks.h
+++ lib/CodeGen/MacroPPCallbacks.h
@@ -0,0 +1,118 @@
+//===--- MacroPPCallbacks.h -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+//  This file defines implementation for the macro preprocessors callbacks.
+//
+//===--===//
+
+#include "clang/Lex/PPCallbacks.h"
+
+namespace llvm {
+class DIMacroFile;
+class DIMacroNode;
+}
+namespace clang {
+class Preprocessor;
+class MacroInfo;
+class CodeGenerator;
+
+class MacroPPCallbacks : public PPCallbacks {
+  /// A pointer to code generator, where debug info generator can be found.
+  CodeGenerator *Gen;
+
+  /// Preprocessor.
+  Preprocessor 
+
+  /// Location of recent included file, used for line number.
+  SourceLocation LastHashLoc;
+
+  /// Counts current number of command line included files, which were entered
+  /// and were not exited yet.
+  int EnteredCommandLineIncludeFiles = 0;
+
+  enum FileScopeStatus {
+NoScope = 0,  // Scope is not initialized yet.
+InitializedScope, // Main file scope is initialized but not set yet.
+BuiltinScope, //  and  file scopes.
+CommandLineIncludeScope,  // Included file, from  file, scope.
+MainFileScope // Main file scope.
+  };
+  FileScopeStatus Status;
+
+  /// Parent contains all entered files that were not exited yet according to
+  /// the inclusion order.
+  llvm::SmallVector Scopes;
+
+  /// Get current DIMacroFile scope.
+  /// \return current DIMacroFile scope or nullptr if there is no such scope.
+  llvm::DIMacroFile *getCurrentScope();
+
+  /// Get current line location or invalid location.
+  /// \param Loc current line location.
+  /// \return current line location \p `Loc`, or invalid location if it's in a
+  /// skipped file scope.
+  SourceLocation getCorrectLocation(SourceLocation Loc);
+
+  /// Use the passed preprocessor to write the macro name and value from the
+  /// given macro info and identifier info into the given \p `Name` and \p
+  /// `Value` output streams.
+  ///
+  /// \param II Identifier info, used to get the Macro name.
+  /// \param MI Macro info, used to get the Macro argumets and values.
+  /// \param PP Preprocessor.
+  /// \param [out] Name Place holder for returned macro name and arguments.
+  /// \param [out] Value Place holder for returned macro value.
+  static void writeMacroDefinition(const IdentifierInfo ,
+   const MacroInfo , Preprocessor ,
+   raw_ostream , raw_ostream );
+
+  /// Update current file scope status to next file scope.
+  void updateStatusToNextScope();
+
+  /// Handle the case when 

Re: [PATCH] D16135: Macro Debug Info support in Clang

[Adrian Prantl via cfe-commits]

> On Feb 8, 2017, at 2:31 PM, David Blaikie  wrote:
> 
> 
> 
> On Wed, Feb 8, 2017 at 2:25 PM Amjad Aboud via Phabricator 
> > wrote:
> aaboud added a comment.
> 
> > How much does the build directory grow?
> >  Is there any noticeable compile time regression?
> 
> I build clang in release mode using GCC, then used that build to build clang 
> in debug mode with "-fstandalone-debug" flag, one time with my changes and 
> another time without my changes.
> 
> Without my changes
> --
> 
> Clang executable Size: 1,565MB
> Build time: 17m57.504s
> 
> Withmy changes
> --
> 
> Clang executable Size: 2,043MB
> Build time:  17m58.008s
> 
> Do, build time is the same, but binary size was increased about 30%.
> However, remember that we are emitting macro debug info in dwarf4 format, 
> once we support emitting it in dwarf5 format the size should be reduced 
> significantly.

That sounds like we shouldn't be enabling it by default at -gdwarf-4; and we 
can revisit that decision for -gdwarf-5 whenever it is ready.

> 
> How are you measuring the build time? Total time for, say "ninja clang" with 
> full parallelism? That'd be hard to measure the actual impact (since it could 
> be the link time or other things are dominating, etc). If you have a reliable 
> way to time (I'm assuming Intel has lots of tools for measuring compiler 
> performance) the compilation, get a sense of the variance, etc (& link time, 
> to get a sense of how much the larger inputs affect linker performance) would 
> be useful.
> 
> But I'm not too fussed if no one else is worried (not sure which, if any 
> platforms other than your own, are planning to turn this on by default so 
> people might not be too invested in it). (wouldn't mind some second opinions, 
> etc)
>  
> 
> > I'm not sure it makes sense to motivate this feature with 
> > "-fstandalone-debug" flag.
> 
> Is "-fmacro-debug" flag sounds good?
> 
> Sounds alright to me.

Personally I find "-fmacro-debug-info" to be more descriptive, but 
"-fmacro-debug" mirrors "-fstandalone-debug", so I'm fine with that too. We 
could also use "-fdebug-macro" for symmetry with "-fdebug-type-section". Does 
GCC have a command line option for this that we could mirror?
Please don't forget to document the new option in the man page and user guide.

-- adrian

>  
> Should we extend DebugInfoKind enumeration to support the debug info macro? 
> Or we should add a new option to CodeGenOptions?
> 
> Separate option - it should be orthogonal to standalone/no-standalone, for 
> example.
>  
> 
> 
> https://reviews.llvm.org/D16135 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16135: Macro Debug Info support in Clang

[David Blaikie via cfe-commits]

On Wed, Feb 8, 2017 at 2:25 PM Amjad Aboud via Phabricator <
revi...@reviews.llvm.org> wrote:

> aaboud added a comment.
>
> > How much does the build directory grow?
> >  Is there any noticeable compile time regression?
>
> I build clang in release mode using GCC, then used that build to build
> clang in debug mode with "-fstandalone-debug" flag, one time with my
> changes and another time without my changes.
>
> Without my changes
> --
>
> Clang executable Size: 1,565MB
> Build time: 17m57.504s
>
> Withmy changes
> --
>
> Clang executable Size: 2,043MB
> Build time:  17m58.008s
>
> Do, build time is the same, but binary size was increased about 30%.
> However, remember that we are emitting macro debug info in dwarf4 format,
> once we support emitting it in dwarf5 format the size should be reduced
> significantly.
>

How are you measuring the build time? Total time for, say "ninja clang"
with full parallelism? That'd be hard to measure the actual impact (since
it could be the link time or other things are dominating, etc). If you have
a reliable way to time (I'm assuming Intel has lots of tools for measuring
compiler performance) the compilation, get a sense of the variance, etc (&
link time, to get a sense of how much the larger inputs affect linker
performance) would be useful.

But I'm not too fussed if no one else is worried (not sure which, if any
platforms other than your own, are planning to turn this on by default so
people might not be too invested in it). (wouldn't mind some second
opinions, etc)


>
> > I'm not sure it makes sense to motivate this feature with
> "-fstandalone-debug" flag.
>
> Is "-fmacro-debug" flag sounds good?
>

Sounds alright to me.


> Should we extend DebugInfoKind enumeration to support the debug info
> macro? Or we should add a new option to CodeGenOptions?
>

Separate option - it should be orthogonal to standalone/no-standalone, for
example.


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

[PATCH] D16135: Macro Debug Info support in Clang

[Amjad Aboud via Phabricator via cfe-commits]

aaboud added a comment.

> How much does the build directory grow?
>  Is there any noticeable compile time regression?

I build clang in release mode using GCC, then used that build to build clang in 
debug mode with "-fstandalone-debug" flag, one time with my changes and another 
time without my changes.

Without my changes
--

Clang executable Size: 1,565MB
Build time: 17m57.504s

Withmy changes
--

Clang executable Size: 2,043MB
Build time:  17m58.008s

Do, build time is the same, but binary size was increased about 30%.
However, remember that we are emitting macro debug info in dwarf4 format, once 
we support emitting it in dwarf5 format the size should be reduced 
significantly.

> I'm not sure it makes sense to motivate this feature with 
> "-fstandalone-debug" flag.

Is "-fmacro-debug" flag sounds good?
Should we extend DebugInfoKind enumeration to support the debug info macro? Or 
we should add a new option to CodeGenOptions?


https://reviews.llvm.org/D16135



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

Re: [PATCH] D16135: Macro Debug Info support in Clang

[David Blaikie via cfe-commits]

On Tue, Feb 7, 2017 at 11:01 AM Amjad Aboud via Phabricator <
revi...@reviews.llvm.org> wrote:

> aaboud added a comment.
>
> In https://reviews.llvm.org/D16135#669416, @aprantl wrote:
>
> > In https://reviews.llvm.org/D16135#669045, @aaboud wrote:
> >
> > > Addressed Adrian last comments.
> > >  Added a LIT tests that covers all the macro kinds:
> > >
> > > 1. built-in (define)
> > > 2. command-line (define, include, undef)
> > > 3. main source (define, include, undef) Checked the above with and
> without PCH include.
> > >
> > >   Notice, that current implementation does not support debug info for
> macro definition and inclusion generated during the PCH file creation. To
> support that, we need to extend the PCH format to preserve this
> information. If you believe this feature is important, I will open a
> bugzilla ticket and we can handle it separately.
> >
> >
> > That would be a good idea and important to fix. I don't think we want
> the the generated code (or the debug info for that matter) to change just
> because the user decides to switch to PCH. Ideally turning on PCH should be
> transparent.
> >
> > I have one final question: What's the impact of this change on, e.g.,
> building clang?
> >
> > - How much does the build directory grow?
> > - Is there any noticeable compile time regression?
>
>
> I will run self-build and come back with answers for these questions.
> I am just wondering, if I build in debug mode, will it use the flag
> "-fstandalone-debug"?
>

I'm not sure it makes sense to motivate this feature with this flag. I'm
going to assume Darwin and FreeBSD don't want to implicitly pick up this
feature because of the default on their platforms which is really to
address a particular limitation in LLDB due to some other debug info.

So this probably needs a separate flag.


> This flag is set by default only for DarwinClang and FreeBSD, is there a
> way to assure it will be used with self-build?
>

You can add flags to your build by modifying your CMakeCache.txt file and
putting them int he CMAKE_CXX_FLAGS/CMAKE_C_FLAGS values (or the specific
ones for teh build mode you'd like them to go with, like the DEBUG build,
most likely)


> Notice that without this flag, there will be no debug info macro generated.
>
>
> https://reviews.llvm.org/D16135
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D16135: Macro Debug Info support in Clang

[Amjad Aboud via Phabricator via cfe-commits]

aaboud added a comment.

In https://reviews.llvm.org/D16135#669416, @aprantl wrote:

> In https://reviews.llvm.org/D16135#669045, @aaboud wrote:
>
> > Addressed Adrian last comments.
> >  Added a LIT tests that covers all the macro kinds:
> >
> > 1. built-in (define)
> > 2. command-line (define, include, undef)
> > 3. main source (define, include, undef) Checked the above with and without 
> > PCH include.
> >
> >   Notice, that current implementation does not support debug info for macro 
> > definition and inclusion generated during the PCH file creation. To support 
> > that, we need to extend the PCH format to preserve this information. If you 
> > believe this feature is important, I will open a bugzilla ticket and we can 
> > handle it separately.
>
>
> That would be a good idea and important to fix. I don't think we want the the 
> generated code (or the debug info for that matter) to change just because the 
> user decides to switch to PCH. Ideally turning on PCH should be transparent.
>
> I have one final question: What's the impact of this change on, e.g., 
> building clang?
>
> - How much does the build directory grow?
> - Is there any noticeable compile time regression?


I will run self-build and come back with answers for these questions.
I am just wondering, if I build in debug mode, will it use the flag 
"-fstandalone-debug"?
This flag is set by default only for DarwinClang and FreeBSD, is there a way to 
assure it will be used with self-build?
Notice that without this flag, there will be no debug info macro generated.


https://reviews.llvm.org/D16135



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

[PATCH] D16135: Macro Debug Info support in Clang

[Adrian Prantl via Phabricator via cfe-commits]

aprantl added a comment.

In https://reviews.llvm.org/D16135#669045, @aaboud wrote:

> Addressed Adrian last comments.
>  Added a LIT tests that covers all the macro kinds:
>
> 1. built-in (define)
> 2. command-line (define, include, undef)
> 3. main source (define, include, undef) Checked the above with and without 
> PCH include.
>
>   Notice, that current implementation does not support debug info for macro 
> definition and inclusion generated during the PCH file creation. To support 
> that, we need to extend the PCH format to preserve this information. If you 
> believe this feature is important, I will open a bugzilla ticket and we can 
> handle it separately.


That would be a good idea and important to fix. I don't think we want the the 
generated code (or the debug info for that matter) to change just because the 
user decides to switch to PCH. Ideally turning on PCH should be transparent.

I have one final question: What's the impact of this change on, e.g., building 
clang?

- How much does the build directory grow?
- Is there any noticeable compile time regression?




Comment at: test/CodeGen/debug-info-macro.c:14
+
+// Please, keep the source lines aligned as below:
+/*Line 15*/ #define D1 1

Alternatively, you may be able to use #line to force an easily recognizable 
line number (unless that messes with the macro debug info, or you can use 
FileCheck's [[@LINE-offset]] feature.


https://reviews.llvm.org/D16135



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

[PATCH] D16135: Macro Debug Info support in Clang

[Amjad Aboud via Phabricator via cfe-commits]

aaboud updated this revision to Diff 87408.
aaboud marked 2 inline comments as done.
aaboud added a comment.

Addressed Adrian last comments.
Added a LIT tests that covers all the macro kinds:

1. built-in (define)
2. command-line (define, include, undef)
3. main source (define, include, undef)

Checked the above with and without PCH include.

Notice, that current implementation does not support debug info for macro 
definition and inclusion generated during the PCH file creation.
To support that, we need to extend the PCH format to preserve this information.
If you believe this feature is important, I will open a bugzilla ticket and we 
can handle it separately.


https://reviews.llvm.org/D16135

Files:
  include/clang/CodeGen/ModuleBuilder.h
  lib/CodeGen/CGDebugInfo.cpp
  lib/CodeGen/CGDebugInfo.h
  lib/CodeGen/CMakeLists.txt
  lib/CodeGen/CodeGenAction.cpp
  lib/CodeGen/MacroPPCallbacks.cpp
  lib/CodeGen/MacroPPCallbacks.h
  lib/CodeGen/ModuleBuilder.cpp
  test/CodeGen/debug-info-global-constant.c
  test/CodeGen/debug-info-macro.c
  test/CodeGen/include/debug-info-macro.h

Index: lib/CodeGen/ModuleBuilder.cpp
===
--- lib/CodeGen/ModuleBuilder.cpp
+++ lib/CodeGen/ModuleBuilder.cpp
@@ -92,6 +92,10 @@
   return M.get();
 }
 
+CGDebugInfo *getCGDebugInfo() {
+  return Builder->getModuleDebugInfo();
+}
+
 llvm::Module *ReleaseModule() {
   return M.release();
 }
@@ -299,6 +303,10 @@
   return static_cast(this)->ReleaseModule();
 }
 
+CGDebugInfo *CodeGenerator::getCGDebugInfo() {
+  return static_cast(this)->getCGDebugInfo();
+}
+
 const Decl *CodeGenerator::GetDeclForMangledName(llvm::StringRef name) {
   return static_cast(this)->GetDeclForMangledName(name);
 }
Index: lib/CodeGen/MacroPPCallbacks.h
===
--- lib/CodeGen/MacroPPCallbacks.h
+++ lib/CodeGen/MacroPPCallbacks.h
@@ -0,0 +1,118 @@
+//===--- MacroPPCallbacks.h -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+//  This file defines implementation for the macro preprocessors callbacks.
+//
+//===--===//
+
+#include "clang/Lex/PPCallbacks.h"
+
+namespace llvm {
+class DIMacroFile;
+class DIMacroNode;
+}
+namespace clang {
+class Preprocessor;
+class MacroInfo;
+class CodeGenerator;
+
+class MacroPPCallbacks : public PPCallbacks {
+  /// A pointer to code generator, where debug info generator can be found.
+  CodeGenerator *Gen;
+
+  /// Preprocessor.
+  Preprocessor 
+
+  /// Location of recent included file, used for line number.
+  SourceLocation LastHashLoc;
+
+  /// Counts current number of command line included files, which were entered
+  /// and were not exited yet.
+  int EnteredCommandLineIncludeFiles = 0;
+
+  enum FileScopeStatus {
+NoScope = 0,  // Scope is not initialized yet.
+InitializedScope, // Main file scope is initialized but not set yet.
+BuiltinScope, //  and  file scopes.
+CommandLineIncludeScope,  // Included file, from  file, scope.
+MainFileScope // Main file scope.
+  };
+  FileScopeStatus Status;
+
+  /// Parent contains all entered files that were not exited yet according to
+  /// the inclusion order.
+  llvm::SmallVector Scopes;
+
+  /// Get current DIMacroFile scope.
+  /// \return current DIMacroFile scope or nullptr if there is no such scope.
+  llvm::DIMacroFile *getCurrentScope();
+
+  /// Get current line location or invalid location.
+  /// \param Loc current line location.
+  /// \return current line location \p `Loc`, or invalid location if it's in a
+  /// skipped file scope.
+  SourceLocation getCorrectLocation(SourceLocation Loc);
+
+  /// Use the passed preprocessor to write the macro name and value from the
+  /// given macro info and identifier info into the given \p `Name` and \p
+  /// `Value` output streams.
+  ///
+  /// \param II Identifier info, used to get the Macro name.
+  /// \param MI Macro info, used to get the Macro argumets and values.
+  /// \param PP Preprocessor.
+  /// \param [out] Name Place holder for returned macro name and arguments.
+  /// \param [out] Value Place holder for returned macro value.
+  static void writeMacroDefinition(const IdentifierInfo ,
+   const MacroInfo , Preprocessor ,
+   raw_ostream , raw_ostream );
+
+  /// Update current file scope status to next file scope.
+  void updateStatusToNextScope();
+
+  /// Handle the case when entering a file.
+  ///
+  /// \param Loc 

[PATCH] D16135: Macro Debug Info support in Clang

[Adrian Prantl via Phabricator via cfe-commits]

aprantl added a comment.

Few more inline comments.
I think the code itself is looking good now. Why doesn't this patch include any 
test cases? I would have expected some that exercise the command line include 
handling, etc., ...




Comment at: lib/CodeGen/MacroPPCallbacks.h:35
+
+  /// Counts current number of command line included files, which was entered
+  /// and was not exited yet.

s/was/were/



Comment at: lib/CodeGen/MacroPPCallbacks.h:37
+  /// and was not exited yet.
+  int CommandIncludeFiles = 0;
+

What about EnteredCommandLineIncludes or EnteredCommandLineIncludeFiles?


https://reviews.llvm.org/D16135



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

[PATCH] D16135: Macro Debug Info support in Clang

[Amjad Aboud via Phabricator via cfe-commits]

aaboud updated this revision to Diff 87020.
aaboud marked 4 inline comments as done.
aaboud added a comment.

Addressed Adrian's comments.
Please, let me know if I missed any.

Also, I improved the code and added explanation about the file inclusion 
structure that is being handled in the macro callbacks.


https://reviews.llvm.org/D16135

Files:
  include/clang/CodeGen/ModuleBuilder.h
  lib/CodeGen/CGDebugInfo.cpp
  lib/CodeGen/CGDebugInfo.h
  lib/CodeGen/CMakeLists.txt
  lib/CodeGen/CodeGenAction.cpp
  lib/CodeGen/MacroPPCallbacks.cpp
  lib/CodeGen/MacroPPCallbacks.h
  lib/CodeGen/ModuleBuilder.cpp
  test/CodeGen/debug-info-global-constant.c

Index: lib/CodeGen/ModuleBuilder.cpp
===
--- lib/CodeGen/ModuleBuilder.cpp
+++ lib/CodeGen/ModuleBuilder.cpp
@@ -92,6 +92,10 @@
   return M.get();
 }
 
+CGDebugInfo *getCGDebugInfo() {
+  return Builder->getModuleDebugInfo();
+}
+
 llvm::Module *ReleaseModule() {
   return M.release();
 }
@@ -299,6 +303,10 @@
   return static_cast(this)->ReleaseModule();
 }
 
+CGDebugInfo *CodeGenerator::getCGDebugInfo() {
+  return static_cast(this)->getCGDebugInfo();
+}
+
 const Decl *CodeGenerator::GetDeclForMangledName(llvm::StringRef name) {
   return static_cast(this)->GetDeclForMangledName(name);
 }
Index: lib/CodeGen/MacroPPCallbacks.h
===
--- lib/CodeGen/MacroPPCallbacks.h
+++ lib/CodeGen/MacroPPCallbacks.h
@@ -0,0 +1,118 @@
+//===--- MacroPPCallbacks.h -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+//  This file defines implementation for the macro preprocessors callbacks.
+//
+//===--===//
+
+#include "clang/Lex/PPCallbacks.h"
+
+namespace llvm {
+class DIMacroFile;
+class DIMacroNode;
+}
+namespace clang {
+class Preprocessor;
+class MacroInfo;
+class CodeGenerator;
+
+class MacroPPCallbacks : public PPCallbacks {
+  /// A pointer to code generator, where debug info generator can be found.
+  CodeGenerator *Gen;
+
+  /// Preprocessor.
+  Preprocessor 
+
+  /// Location of recent included file, used for line number.
+  SourceLocation LastHashLoc;
+
+  /// Counts current number of command line included files, which was entered
+  /// and was not exited yet.
+  int CommandIncludeFiles = 0;
+
+  enum FileScopeStatus {
+NoScope = 0,  // Scope is not initialized yet.
+InitializedScope, // Main file scope is initialized but not set yet.
+BuiltinScope, //  and  file scopes.
+CommandLineScopeIncludes, // Included file, from  file, scope.
+MainFileScope // Main file scope.
+  };
+  FileScopeStatus Status;
+
+  /// Parent contains all entered files that were not exited yet according to
+  /// the inclusion order.
+  llvm::SmallVector Scopes;
+
+  /// Get current DIMacroFile scope.
+  /// \return current DIMacroFile scope or nullptr if there is no such scope.
+  llvm::DIMacroFile *getCurrentScope();
+
+  /// Get current line location or invalid location.
+  /// \param Loc current line location.
+  /// \return current line location \p `Loc`, or invalid location if it's in a
+  /// skipped file scope.
+  SourceLocation getCorrectLocation(SourceLocation Loc);
+
+  /// Use the passed preprocessor to write the macro name and value from the
+  /// given macro info and identifier info into the given \p `Name` and \p
+  /// `Value` output streams.
+  ///
+  /// \param II Identifier info, used to get the Macro name.
+  /// \param MI Macro info, used to get the Macro argumets and values.
+  /// \param PP Preprocessor.
+  /// \param [out] Name Place holder for returned macro name and arguments.
+  /// \param [out] Value Place holder for returned macro value.
+  static void writeMacroDefinition(const IdentifierInfo ,
+   const MacroInfo , Preprocessor ,
+   raw_ostream , raw_ostream );
+
+  /// Update current file scope status to next file scope.
+  void updateStatusToNextScope();
+
+  /// Handle the case when entering a file.
+  ///
+  /// \param Loc Indicates the new location.
+  /// \Return true if file scope status should be updated.
+  void FileEntered(SourceLocation Loc);
+
+  /// Handle the case when exiting a file.
+  ///
+  /// \Return true if file scope status should be updated.
+  void FileExited(SourceLocation Loc);
+
+public:
+  MacroPPCallbacks(CodeGenerator *Gen, Preprocessor );
+
+  /// Callback invoked whenever a source file is entered or exited.
+  ///
+  /// \param Loc Indicates the new 

[PATCH] D16135: Macro Debug Info support in Clang

[Amjad Aboud via Phabricator via cfe-commits]

aaboud added a comment.

Thanks Adrian for the fast response.
See some answers below.

I will update the code and upload a new patch soon.




Comment at: lib/CodeGen/MacroPPCallbacks.cpp:154
+StringRef SearchPath, StringRef RelativePath, const Module *Imported) {
+  // Only care about "include" directives.
+  if (!IncludeTok.is(tok::identifier))

aprantl wrote:
> aaboud wrote:
> > aprantl wrote:
> > > #include as opposed to #import? If so, why?
> > I am not familiar with "#import" keyword.
> > 1. Is it handled differently by clang that #include?
> > 2. Do we need to generate macro debug info for it?
> > 
> > The reason I have this switch below, is that I am not sure if we call this 
> > "InclusionDirective" callback for identifiers that are not "#include".
> > 
> > However, I see no harm of updating the LastHashLoc every time we enter this 
> > function. The value will not be used unless it is set just before calling 
> > "FileChanged"callback.
> #import comes from Objective-C. It is equivalent to #include + #pragma once.
> I'm curious whether this comment means that it isn't handled?
I will remove the switch and update the LastHashLoc unconditionally.
I did not check Objective-C debug info, but I assume it will work fine once I 
change these lines.



Comment at: lib/CodeGen/MacroPPCallbacks.cpp:88
+  // Invalid location represents line zero.
+  return SourceLocation();
+}

aprantl wrote:
> I think the entire function can be replaced with
> ```
> if ((Status == MainFileScope) ||
> (Status == CommandLineScopeIncludes && CommandIncludeFiles))
>   return Loc;
> return SourceLocation();
> ```
Sure, will do.
The reason I have a switch, because I had more cases during the implementation 
that I get rid of.



Comment at: lib/CodeGen/MacroPPCallbacks.cpp:100
+  bool CreateMacroFile = false;
+  bool PopScope = false;
+  SourceLocation LineLoc = getCorrectLocation(LastHashLoc);

aprantl wrote:
> Could you try splitting this function up into two functions?
> 
> MacroPPCallbacks::FileEntered()
> MacroPPCallbacks::FileExited()
> 
> and the common code factored out into a helper? The control flow here seems 
> unnecessarily complex with all the flags.
Will try restructure.
The way I implemented this is by having stages (parsing levels).
The control flow is to identify the current stage and decide how to continue 
from there.

I will see what I can do to make the code much clear.



Comment at: lib/CodeGen/MacroPPCallbacks.cpp:177
+  llvm::raw_string_ostream Value(ValueBuffer);
+  calculatMacroDefinition(*Id, *MD->getMacroInfo(), PP, Name, Value);
+  Gen->getCGDebugInfo()->CreateMacro(getCurrentScope(),

aprantl wrote:
> calculat*e*MacroDefinition
> (is there a better word than calculate?)
How about:
GenerateMacroDefinition
PrintMacroDefinition
ParseMacroDefinition

Do you think any of the above would be better choice?


https://reviews.llvm.org/D16135



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

[PATCH] D16135: Macro Debug Info support in Clang

[Adrian Prantl via Phabricator via cfe-commits]

aprantl added a comment.

Thanks! Couple more inline comments.




Comment at: lib/CodeGen/CGDebugInfo.h:415
 
+  /// Get macro debug info.
+  llvm::DIMacro *CreateMacro(llvm::DIMacroFile *Parent, unsigned MType,

It would be good to be a bit more descriptive here. If the comment just repeats 
the name of the function it is not worth keeping,
/// Create debug info for a macro (definition/use/?).



Comment at: lib/CodeGen/CGDebugInfo.h:420
+
+  /// Get temporary macro file debug info.
+  llvm::DIMacroFile *CreateTempMacroFile(llvm::DIMacroFile *Parent,

E.g.,
/// Create debug info for a file referenced by an #include directive.



Comment at: lib/CodeGen/MacroPPCallbacks.cpp:44
+if (MI.isGNUVarargs())
+  Name << "..."; // #define foo(x...)
+

aaboud wrote:
> aprantl wrote:
> > LLVM style prefers comments to be full sentences and above the code.
> As I said above, this code is taken from 
> "lib\Frontend\PrintPreprocessedOutput.cpp"
> Also, I ran clang-format on it, and it did not suggest to change this line.
> 
> But, I do not mind fixing this, if it is the preferred comment style.
I think it would be a good idea to clean this up. Feel free to either do this 
now or in a separate commit.



Comment at: lib/CodeGen/MacroPPCallbacks.cpp:154
+StringRef SearchPath, StringRef RelativePath, const Module *Imported) {
+  // Only care about "include" directives.
+  if (!IncludeTok.is(tok::identifier))

aaboud wrote:
> aprantl wrote:
> > #include as opposed to #import? If so, why?
> I am not familiar with "#import" keyword.
> 1. Is it handled differently by clang that #include?
> 2. Do we need to generate macro debug info for it?
> 
> The reason I have this switch below, is that I am not sure if we call this 
> "InclusionDirective" callback for identifiers that are not "#include".
> 
> However, I see no harm of updating the LastHashLoc every time we enter this 
> function. The value will not be used unless it is set just before calling 
> "FileChanged"callback.
#import comes from Objective-C. It is equivalent to #include + #pragma once.
I'm curious whether this comment means that it isn't handled?



Comment at: lib/CodeGen/MacroPPCallbacks.cpp:88
+  // Invalid location represents line zero.
+  return SourceLocation();
+}

I think the entire function can be replaced with
```
if ((Status == MainFileScope) ||
(Status == CommandLineScopeIncludes && CommandIncludeFiles))
  return Loc;
return SourceLocation();
```



Comment at: lib/CodeGen/MacroPPCallbacks.cpp:100
+  bool CreateMacroFile = false;
+  bool PopScope = false;
+  SourceLocation LineLoc = getCorrectLocation(LastHashLoc);

Could you try splitting this function up into two functions?

MacroPPCallbacks::FileEntered()
MacroPPCallbacks::FileExited()

and the common code factored out into a helper? The control flow here seems 
unnecessarily complex with all the flags.



Comment at: lib/CodeGen/MacroPPCallbacks.cpp:177
+  llvm::raw_string_ostream Value(ValueBuffer);
+  calculatMacroDefinition(*Id, *MD->getMacroInfo(), PP, Name, Value);
+  Gen->getCGDebugInfo()->CreateMacro(getCurrentScope(),

calculat*e*MacroDefinition
(is there a better word than calculate?)


https://reviews.llvm.org/D16135



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

[PATCH] D16135: Macro Debug Info support in Clang

[Amjad Aboud via Phabricator via cfe-commits]

aaboud updated this revision to Diff 86669.
aaboud marked an inline comment as done.
aaboud added a comment.

Address Adrian and Richard comments.

Please, review the new patch and let me know if I need to change anything else.

Thanks,
Amjad


https://reviews.llvm.org/D16135

Files:
  include/clang/CodeGen/ModuleBuilder.h
  lib/CodeGen/CGDebugInfo.cpp
  lib/CodeGen/CGDebugInfo.h
  lib/CodeGen/CMakeLists.txt
  lib/CodeGen/CodeGenAction.cpp
  lib/CodeGen/MacroPPCallbacks.cpp
  lib/CodeGen/MacroPPCallbacks.h
  lib/CodeGen/ModuleBuilder.cpp
  test/CodeGen/debug-info-global-constant.c

Index: lib/CodeGen/ModuleBuilder.cpp
===
--- lib/CodeGen/ModuleBuilder.cpp
+++ lib/CodeGen/ModuleBuilder.cpp
@@ -92,6 +92,10 @@
   return M.get();
 }
 
+CGDebugInfo *getCGDebugInfo() {
+  return Builder->getModuleDebugInfo();
+}
+
 llvm::Module *ReleaseModule() {
   return M.release();
 }
@@ -299,6 +303,10 @@
   return static_cast(this)->ReleaseModule();
 }
 
+CGDebugInfo *CodeGenerator::getCGDebugInfo() {
+  return static_cast(this)->getCGDebugInfo();
+}
+
 const Decl *CodeGenerator::GetDeclForMangledName(llvm::StringRef name) {
   return static_cast(this)->GetDeclForMangledName(name);
 }
Index: lib/CodeGen/MacroPPCallbacks.h
===
--- lib/CodeGen/MacroPPCallbacks.h
+++ lib/CodeGen/MacroPPCallbacks.h
@@ -0,0 +1,107 @@
+//===--- MacroPPCallbacks.h -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+//  This file defines implementation for the macro preprocessors callbacks.
+//
+//===--===//
+
+#include "clang/Lex/PPCallbacks.h"
+
+namespace llvm {
+class DIMacroFile;
+class DIMacroNode;
+}
+namespace clang {
+class Preprocessor;
+class MacroInfo;
+class CodeGenerator;
+
+class MacroPPCallbacks : public PPCallbacks {
+  /// A pointer to code generator, where debug info generator can be found.
+  CodeGenerator *Gen;
+
+  /// Preprocessor.
+  Preprocessor 
+
+  /// Location of recent included file, used for line number.
+  SourceLocation LastHashLoc;
+
+  /// Location of main file, used for file info.
+  SourceLocation MainFileLoc;
+
+  /// Counts current number of command line included files, which was entered
+  /// and was not exited yet.
+  int CommandIncludeFiles = 0;
+
+  enum FileScopeStatus {
+NoScope,  // Scope is not initialized yet.
+InitializedScope, // Main file scope is initialized but not set yet.
+BuiltinScope, //  file scope.
+CommandLineScope, //  file scope.
+CommandLineScopeIncludes, // Included file, from  file, scope.
+MainFileScope // Main file scope.
+  };
+  FileScopeStatus Status;
+
+  /// Parent contains all entered files that were not exited yet according to
+  /// the inclusion order.
+  llvm::SmallVector Scopes;
+
+  /// Get current DIMacroFile scope.
+  /// \return current DIMacroFile scope or nullptr if there is no such scope.
+  llvm::DIMacroFile *getCurrentScope();
+
+  /// Get current line location or invalid location.
+  /// \param Loc current line location.
+  /// \return current line location \p `Loc`, or invalid location if it's in a
+  /// skipped file scope.
+  SourceLocation getCorrectLocation(SourceLocation Loc);
+
+  /// Use the passed preprocessor to calculate the macro name and value from
+  /// the given macro info and identifier info.
+  ///
+  /// \param II Identifier info, used to get the Macro name.
+  /// \param MI Macro info, used to get the Macro argumets and values.
+  /// \param PP Preprocessor.
+  /// \param [out] Name Place holder for returned macro name and arguments.
+  /// \param [out] Value Place holder for returned macro value.
+  static void calculatMacroDefinition(const IdentifierInfo ,
+  const MacroInfo , Preprocessor ,
+  raw_ostream , raw_ostream );
+
+public:
+  MacroPPCallbacks(CodeGenerator *Gen, Preprocessor );
+
+  /// Callback invoked whenever a source file is entered or exited.
+  ///
+  /// \param Loc Indicates the new location.
+  /// \param PrevFID the file that was exited if \p Reason is ExitFile.
+  void FileChanged(SourceLocation Loc, FileChangeReason Reason,
+   SrcMgr::CharacteristicKind FileType,
+   FileID PrevFID = FileID()) override;
+
+  /// Callback invoked whenever a directive (#xxx) is processed.
+  void InclusionDirective(SourceLocation HashLoc, const Token ,
+  StringRef 

[PATCH] D16135: Macro Debug Info support in Clang

[Amjad Aboud via Phabricator via cfe-commits]

aaboud marked 7 inline comments as done.
aaboud added a comment.

Thanks Adrian and Richard for the comments, I appreciate that.
I am uploading a new patch that address most of the comments.

Adrian, I answered some of your comments below.

Thanks,
Amjad




Comment at: lib/CodeGen/MacroPPCallbacks.cpp:31
+  MacroInfo::arg_iterator AI = MI.arg_begin(), E = MI.arg_end();
+  for (; AI + 1 != E; ++AI) {
+Name << (*AI)->getName();

aprantl wrote:
> std::next() ?
I am not that satisfied with this function, and I hope we can do it in a better 
way.
This function is based on a code I took from this a static function in file 
"lib\Frontend\PrintPreprocessedOutput.cpp":

```
/// PrintMacroDefinition - Print a macro definition in a form that will be
/// properly accepted back as a definition.
static void PrintMacroDefinition(const IdentifierInfo , const MacroInfo ,
 Preprocessor , raw_ostream ) {
```



Comment at: lib/CodeGen/MacroPPCallbacks.cpp:44
+if (MI.isGNUVarargs())
+  Name << "..."; // #define foo(x...)
+

aprantl wrote:
> LLVM style prefers comments to be full sentences and above the code.
As I said above, this code is taken from 
"lib\Frontend\PrintPreprocessedOutput.cpp"
Also, I ran clang-format on it, and it did not suggest to change this line.

But, I do not mind fixing this, if it is the preferred comment style.



Comment at: lib/CodeGen/MacroPPCallbacks.cpp:154
+StringRef SearchPath, StringRef RelativePath, const Module *Imported) {
+  // Only care about "include" directives.
+  if (!IncludeTok.is(tok::identifier))

aprantl wrote:
> #include as opposed to #import? If so, why?
I am not familiar with "#import" keyword.
1. Is it handled differently by clang that #include?
2. Do we need to generate macro debug info for it?

The reason I have this switch below, is that I am not sure if we call this 
"InclusionDirective" callback for identifiers that are not "#include".

However, I see no harm of updating the LastHashLoc every time we enter this 
function. The value will not be used unless it is set just before calling 
"FileChanged"callback.



Comment at: lib/CodeGen/ModuleBuilder.cpp:68
 SmallVector DeferredInlineMethodDefinitions;
+CGDebugInfo *DebugInfoRef;
 

aprantl wrote:
> What does Ref stand for?
Ref - stands for reference, I think I do not need the "Ref" prefix for this 
member, only for the get function.
But, I will follow Richards suggestion that will eliminate the need for this 
member.


https://reviews.llvm.org/D16135



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

[PATCH] D16135: Macro Debug Info support in Clang

[Richard Smith via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: include/clang/CodeGen/ModuleBuilder.h:71
+  ///
+  /// This methods can be called before initializing the CGDebugInfo calss.
+  /// But the returned value should not be used until after initialization.

Typo 'calss'



Comment at: include/clang/CodeGen/ModuleBuilder.h:72
+  /// This methods can be called before initializing the CGDebugInfo calss.
+  /// But the returned value should not be used until after initialization.
+  /// It is caller responsibility to validate that the place holder was

What is the returned value in that case? A reference to a null pointer?

Generally, I'm not particularly happy with this approach of exposing a 
reference to an internal pointer as the way of propagating the `CGDebugInfo` to 
the macro generator as needed. Instead, how about giving the `MacroPPCallbacks` 
object a pointer to the `CodeGenerator` itself, and having it ask the 
`CodeGenModule` for the debug info object when it needs it?



Comment at: include/clang/CodeGen/ModuleBuilder.h:73
+  /// But the returned value should not be used until after initialization.
+  /// It is caller responsibility to validate that the place holder was
+  /// initialized before start using it.

caller -> the caller's



Comment at: include/clang/CodeGen/ModuleBuilder.h:74
+  /// It is caller responsibility to validate that the place holder was
+  /// initialized before start using it.
+  CodeGen::CGDebugInfo *::getModuleDebugInfoRef();

start using -> starting to use



Comment at: lib/CodeGen/MacroPPCallbacks.cpp:1
+//===--- MacroPPCallbacks.h -*- C++ 
-*-===//
+//

Wrong file header (should be .cpp, and you don't need the "-*- C++ -*-" in a 
.cpp file.


https://reviews.llvm.org/D16135



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

[PATCH] D16135: Macro Debug Info support in Clang

[Adrian Prantl via Phabricator via cfe-commits]

aprantl added inline comments.



Comment at: lib/CodeGen/MacroPPCallbacks.cpp:62
+: DebugInfo(DI), PP(PP), FirstInclude(false), FirstIncludeDone(false),
+  CommandIncludeFiles(0), SkipFiles(2) {
+  Parents.push_back(nullptr);

aprantl wrote:
> Comment why SkipFiles is initialized to 2?
Thanks. I think this comment should now go into the header and be promoted to 
the Docygen comment for the CommandIncludeFiles. Also consider doing 
`CommandIncludeFiles = 0` in the class definition.



Comment at: lib/CodeGen/MacroPPCallbacks.cpp:31
+  MacroInfo::arg_iterator AI = MI.arg_begin(), E = MI.arg_end();
+  for (; AI + 1 != E; ++AI) {
+Name << (*AI)->getName();

std::next() ?



Comment at: lib/CodeGen/MacroPPCallbacks.cpp:44
+if (MI.isGNUVarargs())
+  Name << "..."; // #define foo(x...)
+

LLVM style prefers comments to be full sentences and above the code.



Comment at: lib/CodeGen/MacroPPCallbacks.cpp:83
+  break;
+  // Fall though!
+  case MainFileScope:

I think there is an LLVM_FALLTHROUGH macro (or something to that end)



Comment at: lib/CodeGen/MacroPPCallbacks.cpp:154
+StringRef SearchPath, StringRef RelativePath, const Module *Imported) {
+  // Only care about "include" directives.
+  if (!IncludeTok.is(tok::identifier))

#include as opposed to #import? If so, why?



Comment at: lib/CodeGen/ModuleBuilder.cpp:68
 SmallVector DeferredInlineMethodDefinitions;
+CGDebugInfo *DebugInfoRef;
 

What does Ref stand for?


https://reviews.llvm.org/D16135



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

[PATCH] D16135: Macro Debug Info support in Clang

[Michael Kuperstein via Phabricator via cfe-commits]

mkuper resigned from this revision.
mkuper added a comment.

I admit it would be nice if someone reviewed this, but I'm really the wrong 
person for this code area. :-)


https://reviews.llvm.org/D16135



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

[PATCH] D16135: Macro Debug Info support in Clang

[Amjad Aboud via Phabricator via cfe-commits]

aaboud added a comment.

Kindly reminder, I am still waiting for a review on this new patch.

Thanks


https://reviews.llvm.org/D16135



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

[PATCH] D16135: Macro Debug Info support in Clang

[Amjad Aboud via Phabricator via cfe-commits]

aaboud added a comment.

Richard, can you please review the new patch?


https://reviews.llvm.org/D16135



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

[PATCH] D16135: Macro Debug Info support in Clang

[Amjad Aboud via Phabricator via cfe-commits]

aaboud updated this revision to Diff 82696.
aaboud added a comment.
Herald added subscribers: mgorny, nemanjai.

Improved code based on Richard's comments.
Removed the change to ASTConsumer, now it does not need to know anything about 
PP consumer.
However, I still needed to change the CodeGenerator ASTConsumer to return the 
CGDebugInfo class.

Richard, please review the new patch.
This time I made sure to add the "MacroPPCallbacks.*" new files.


https://reviews.llvm.org/D16135

Files:
  include/clang/CodeGen/ModuleBuilder.h
  lib/CodeGen/CGDebugInfo.cpp
  lib/CodeGen/CGDebugInfo.h
  lib/CodeGen/CMakeLists.txt
  lib/CodeGen/CodeGenAction.cpp
  lib/CodeGen/MacroPPCallbacks.cpp
  lib/CodeGen/MacroPPCallbacks.h
  lib/CodeGen/ModuleBuilder.cpp
  test/CodeGen/debug-info-global-constant.c

Index: lib/CodeGen/ModuleBuilder.cpp
===
--- lib/CodeGen/ModuleBuilder.cpp
+++ lib/CodeGen/ModuleBuilder.cpp
@@ -65,6 +65,7 @@
 
   private:
 SmallVector DeferredInlineMethodDefinitions;
+CGDebugInfo *DebugInfoRef;
 
   public:
 CodeGeneratorImpl(DiagnosticsEngine , llvm::StringRef ModuleName,
@@ -74,7 +75,8 @@
   CoverageSourceInfo *CoverageInfo = nullptr)
 : Diags(diags), Ctx(nullptr), HeaderSearchOpts(HSO),
   PreprocessorOpts(PPO), CodeGenOpts(CGO), HandlingTopLevelDecls(0),
-  CoverageInfo(CoverageInfo), M(new llvm::Module(ModuleName, C)) {
+  CoverageInfo(CoverageInfo), M(new llvm::Module(ModuleName, C)),
+  DebugInfoRef(nullptr) {
   C.setDiscardValueNames(CGO.DiscardValueNames);
 }
 
@@ -92,6 +94,10 @@
   return M.get();
 }
 
+CGDebugInfo *() {
+  return DebugInfoRef;
+}
+
 llvm::Module *ReleaseModule() {
   return M.release();
 }
@@ -124,6 +130,9 @@
PreprocessorOpts, CodeGenOpts,
*M, Diags, CoverageInfo));
 
+  // Initialize the place holder for the CGDebugInfo.
+  DebugInfoRef = Builder->getModuleDebugInfo();
+
   for (auto & : CodeGenOpts.DependentLibraries)
 Builder->AddDependentLib(Lib);
   for (auto & : CodeGenOpts.LinkerOptions)
@@ -299,6 +308,10 @@
   return static_cast(this)->ReleaseModule();
 }
 
+CGDebugInfo *::getModuleDebugInfoRef() {
+  return static_cast(this)->getModuleDebugInfoRef();
+}
+
 const Decl *CodeGenerator::GetDeclForMangledName(llvm::StringRef name) {
   return static_cast(this)->GetDeclForMangledName(name);
 }
Index: lib/CodeGen/MacroPPCallbacks.h
===
--- lib/CodeGen/MacroPPCallbacks.h
+++ lib/CodeGen/MacroPPCallbacks.h
@@ -0,0 +1,110 @@
+//===--- MacroPPCallbacks.h -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+//  This file defines implementation for the macro preprocessors callbacks.
+//
+//===--===//
+
+#include "clang/Lex/PPCallbacks.h"
+
+namespace llvm {
+class DIMacroFile;
+class DIMacroNode;
+}
+namespace clang {
+class Preprocessor;
+class MacroInfo;
+
+namespace CodeGen {
+class CGDebugInfo;
+}
+
+class MacroPPCallbacks : public PPCallbacks {
+  /// A reference pointer to debug info code generator.
+  CodeGen::CGDebugInfo *
+
+  /// Preprocessor.
+  Preprocessor 
+
+  /// Location of recent included file, used for line number.
+  SourceLocation LastHashLoc;
+
+  /// Location of main file, used for file info.
+  SourceLocation MainFileLoc;
+
+  /// Counts current number of command line included files, which was entered
+  /// and was not exited yet.
+  int CommandIncludeFiles;
+
+  enum FileScopeStatus {
+NoScope,  // Scope is not initialized yet.
+InitializedScope, // Main file scope is initialized but not set yet.
+BuiltinScope, //  file scope.
+CommandLineScope, //  file scope.
+CommandLineScopeIncludes, // Included file, from  file, scope.
+MainFileScope // Main file scope.
+  };
+  FileScopeStatus Status;
+
+  /// Parent contains all entered files that were not exited yet according to
+  /// the inclusion order.
+  llvm::SmallVector Scopes;
+
+  /// Get current DIMacroFile scope.
+  /// \return current DIMacroFile scope or nullptr if there is no such scope.
+  llvm::DIMacroFile *getCurrentScope();
+
+  /// Get current line location or invalid location.
+  /// \param Loc current line location.
+  /// \return current line location \p `Loc`, or invalid location if it's in a
+  /// skipped file scope.
+  SourceLocation 

[PATCH] D16135: Macro Debug Info support in Clang

[Richard Smith via cfe-commits]

rsmith added inline comments.



Comment at: include/clang/AST/ASTConsumer.h:163
+  /// The caller takes ownership on the returned pointer.
+  virtual std::unique_ptr 
CreatePreprocessorCallbacks(Preprocessor );
 };

aaboud wrote:
> Richard,
> I know that you suggested not to introduce the Preprocessor in anyway to the 
> ASTConsumer.
> However, as I could not understand the other way to support macros in Clang 
> without applying a huge redesign, I decided to upload the code I suggested in 
> the proposal.
> http://lists.llvm.org/pipermail/llvm-dev/2015-November/092449.html
> 
> If you still think that this approach is not the right one, I would 
> appreciate it if you can explain again how to implement the macro support 
> differently.
Don't add this to `ASTConsumer`; an AST consumer and a PP consumer are two 
largely separate ideas. Instead, you could create and register a `PPCallbacks` 
object directly from `CodeGenAction::CreateASTConsumer`. In fact, if you look 
there, a `PPCallbacks` subclass is already registered (for code coverage); you 
can do something similar to register your `MacroPPCallbacks` object.



Comment at: lib/CodeGen/CMakeLists.txt:76
   ItaniumCXXABI.cpp
+  MacroPPCallbacks.cpp
   MicrosoftCXXABI.cpp

This file is missing from the diff.



Comment at: lib/CodeGen/CodeGenModule.cpp:28
 #include "CoverageMappingGen.h"
+#include "MacroPPCallbacks.h"
 #include "TargetInfo.h"

This file is missing from the diff.


https://reviews.llvm.org/D16135



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

Re: [PATCH] D16135: Macro Debug Info support in Clang

[Amjad Aboud via cfe-commits]

aaboud added reviewers: erichkeane, bwyma.
aaboud updated this revision to Diff 65551.
aaboud added a comment.

Updated patch to top of trunk (r276746) - Thanks to Ranjeet Singh.

Please, provide your feedback.


https://reviews.llvm.org/D16135

Files:
  include/clang/AST/ASTConsumer.h
  lib/AST/ASTConsumer.cpp
  lib/CodeGen/CGDebugInfo.cpp
  lib/CodeGen/CGDebugInfo.h
  lib/CodeGen/CMakeLists.txt
  lib/CodeGen/CodeGenAction.cpp
  lib/CodeGen/CodeGenModule.cpp
  lib/CodeGen/CodeGenModule.h
  lib/CodeGen/ModuleBuilder.cpp
  lib/Parse/ParseAST.cpp

Index: lib/Parse/ParseAST.cpp
===
--- lib/Parse/ParseAST.cpp
+++ lib/Parse/ParseAST.cpp
@@ -127,6 +127,12 @@
   new Parser(S.getPreprocessor(), S, SkipFunctionBodies));
   Parser  = *ParseOP.get();
 
+  std::unique_ptr Callbacks =
+  Consumer->CreatePreprocessorCallbacks(S.getPreprocessor());
+  if (Callbacks) {
+S.getPreprocessor().addPPCallbacks(std::move(Callbacks));
+  }
+
   llvm::CrashRecoveryContextCleanupRegistrar
   CleanupPrettyStack(llvm::SavePrettyStackState());
   PrettyStackTraceParserEntry CrashInfo(P);
Index: lib/CodeGen/ModuleBuilder.cpp
===
--- lib/CodeGen/ModuleBuilder.cpp
+++ lib/CodeGen/ModuleBuilder.cpp
@@ -20,6 +20,7 @@
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Frontend/CodeGenOptions.h"
+#include "clang/Lex/PPCallbacks.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/LLVMContext.h"
@@ -130,6 +131,11 @@
 Builder->AppendLinkerOptions(Opt);
 }
 
+std::unique_ptr
+CreatePreprocessorCallbacks(Preprocessor ) override {
+  return Builder->createPreprocessorCallbacks(PP);
+}
+
 void HandleCXXStaticMemberVarInstantiation(VarDecl *VD) override {
   if (Diags.hasErrorOccurred())
 return;
Index: lib/CodeGen/CodeGenModule.cpp
===
--- lib/CodeGen/CodeGenModule.cpp
+++ lib/CodeGen/CodeGenModule.cpp
@@ -25,6 +25,7 @@
 #include "CodeGenPGO.h"
 #include "CodeGenTBAA.h"
 #include "CoverageMappingGen.h"
+#include "MacroPPCallbacks.h"
 #include "TargetInfo.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/CharUnits.h"
@@ -204,6 +205,16 @@
   CUDARuntime.reset(CreateNVCUDARuntime(*this));
 }
 
+std::unique_ptr
+CodeGenModule::createPreprocessorCallbacks(Preprocessor ) {
+  // Enable generating macro debug info only in FullDebugInfo mode.
+  if (CodeGenOpts.getDebugInfo() < codegenoptions::FullDebugInfo ||
+  !getModuleDebugInfo())
+return nullptr;
+
+  return llvm::make_unique(*getModuleDebugInfo(), PP);
+}
+
 void CodeGenModule::addReplacement(StringRef Name, llvm::Constant *C) {
   Replacements[Name] = C;
 }
Index: lib/CodeGen/CMakeLists.txt
===
--- lib/CodeGen/CMakeLists.txt
+++ lib/CodeGen/CMakeLists.txt
@@ -73,6 +73,7 @@
   CodeGenTypes.cpp
   CoverageMappingGen.cpp
   ItaniumCXXABI.cpp
+  MacroPPCallbacks.cpp
   MicrosoftCXXABI.cpp
   ModuleBuilder.cpp
   ObjectFilePCHContainerOperations.cpp
Index: lib/CodeGen/CGDebugInfo.h
===
--- lib/CodeGen/CGDebugInfo.h
+++ lib/CodeGen/CGDebugInfo.h
@@ -386,6 +386,16 @@
 
   void completeTemplateDefinition(const ClassTemplateSpecializationDecl );
 
+  /// Get Macro debug info.
+  llvm::DIMacro *CreateMacro(llvm::DIMacroFile *Parent, unsigned MType,
+ SourceLocation LineLoc, StringRef Name,
+ StringRef Value);
+
+  /// Get Macro File debug info.
+  llvm::DIMacroFile *CreateMacroFile(llvm::DIMacroFile *Parent,
+ SourceLocation LineLoc,
+ SourceLocation FileLoc);
+
 private:
   /// Emit call to llvm.dbg.declare for a variable declaration.
   void EmitDeclare(const VarDecl *decl, llvm::Value *AI,
Index: lib/CodeGen/CodeGenAction.cpp
===
--- lib/CodeGen/CodeGenAction.cpp
+++ lib/CodeGen/CodeGenAction.cpp
@@ -222,6 +222,11 @@
   Gen->HandleVTable(RD);
 }
 
+std::unique_ptr
+CreatePreprocessorCallbacks(Preprocessor ) override {
+  return Gen->CreatePreprocessorCallbacks(PP);
+}
+
 static void InlineAsmDiagHandler(const llvm::SMDiagnostic ,void *Context,
  unsigned LocCookie) {
   SourceLocation Loc = SourceLocation::getFromRawEncoding(LocCookie);
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -2264,6 +2264,22 @@
 FullName);
 }
 
+llvm::DIMacro *CGDebugInfo::CreateMacro(llvm::DIMacroFile *Parent,
+

RE: [PATCH] D16135: Macro Debug Info support in Clang

[Aboud, Amjad via cfe-commits]

Thanks Ranjeet,
Indeed I was planning to update the patches to top of trunk and reload them, so 
if you can save me this trouble I would appreciate that.
Once we upload the updated patches I would like to ping the community and ask 
for feedback.

Thanks,
Amjad 
> -Original Message-
> From: Ranjeet Singh [mailto:ranjeet.si...@arm.com]
> Sent: Friday, July 22, 2016 20:16
> To: Aboud, Amjad <amjad.ab...@intel.com>; rich...@metafoo.co.uk;
> paul.robin...@sony.com; apra...@apple.com
> Cc: ranjeet.si...@arm.com; cfe-commits@lists.llvm.org
> Subject: Re: [PATCH] D16135: Macro Debug Info support in Clang
> 
> rs added a subscriber: rs.
> rs added a comment.
> 
> Hi Amjad,
> 
> are you still planning on getting this patch and
> https://reviews.llvm.org/D16077 committed ? It looks like these two patches
> are final pieces in the puzzle to get macro information in the DWARF debug
> output.
> 
> I've downloaded the diffs and applied them myself on my local checkout and
> they seem to work fine. If you would like me to upload the rebased patches
> onto phabricator to save you the trouble of having to the fix conflicts
> downstream then let me know.
> 
> Thanks,
> Ranjeet
> 
> 
> https://reviews.llvm.org/D16135
> 
> 

-
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16135: Macro Debug Info support in Clang

[Ranjeet Singh via cfe-commits]

rs added a subscriber: rs.
rs added a comment.

Hi Amjad,

are you still planning on getting this patch and 
https://reviews.llvm.org/D16077 committed ? It looks like these two patches are 
final pieces in the puzzle to get macro information in the DWARF debug output.

I've downloaded the diffs and applied them myself on my local checkout and they 
seem to work fine. If you would like me to upload the rebased patches onto 
phabricator to save you the trouble of having to the fix conflicts downstream 
then let me know.

Thanks,
Ranjeet


https://reviews.llvm.org/D16135



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

Re: [PATCH] D16135: Macro Debug Info support in Clang

[Amjad Aboud via cfe-commits]

aaboud updated this revision to Diff 47997.
aaboud marked an inline comment as done.
aaboud added a comment.

Applied Adrian comment.


http://reviews.llvm.org/D16135

Files:
  include/clang/AST/ASTConsumer.h
  lib/AST/ASTConsumer.cpp
  lib/CodeGen/CGDebugInfo.cpp
  lib/CodeGen/CGDebugInfo.h
  lib/CodeGen/CMakeLists.txt
  lib/CodeGen/CodeGenAction.cpp
  lib/CodeGen/CodeGenModule.cpp
  lib/CodeGen/CodeGenModule.h
  lib/CodeGen/MacroPPCallbacks.cpp
  lib/CodeGen/MacroPPCallbacks.h
  lib/CodeGen/ModuleBuilder.cpp
  lib/Parse/ParseAST.cpp

Index: lib/Parse/ParseAST.cpp
===
--- lib/Parse/ParseAST.cpp
+++ lib/Parse/ParseAST.cpp
@@ -128,6 +128,12 @@
   new Parser(S.getPreprocessor(), S, SkipFunctionBodies));
   Parser  = *ParseOP.get();
 
+  std::unique_ptr Callbacks =
+  Consumer->CreatePreprocessorCallbacks(S.getPreprocessor());
+  if (Callbacks) {
+S.getPreprocessor().addPPCallbacks(std::move(Callbacks));
+  }
+
   llvm::CrashRecoveryContextCleanupRegistrar
   CleanupPrettyStack(llvm::SavePrettyStackState());
   PrettyStackTraceParserEntry CrashInfo(P);
Index: lib/CodeGen/ModuleBuilder.cpp
===
--- lib/CodeGen/ModuleBuilder.cpp
+++ lib/CodeGen/ModuleBuilder.cpp
@@ -20,6 +20,7 @@
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Frontend/CodeGenOptions.h"
+#include "clang/Lex/PPCallbacks.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/LLVMContext.h"
@@ -234,6 +235,11 @@
 void HandleDependentLibrary(llvm::StringRef Lib) override {
   Builder->AddDependentLib(Lib);
 }
+
+std::unique_ptr
+CreatePreprocessorCallbacks(Preprocessor ) override {
+  return Builder->createPreprocessorCallbacks(PP);
+}
   };
 }
 
Index: lib/CodeGen/CodeGenModule.cpp
===
--- lib/CodeGen/CodeGenModule.cpp
+++ lib/CodeGen/CodeGenModule.cpp
@@ -24,6 +24,7 @@
 #include "CodeGenPGO.h"
 #include "CodeGenTBAA.h"
 #include "CoverageMappingGen.h"
+#include "MacroPPCallbacks.h"
 #include "TargetInfo.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/CharUnits.h"
@@ -208,6 +209,16 @@
   CUDARuntime = CreateNVCUDARuntime(*this);
 }
 
+std::unique_ptr
+CodeGenModule::createPreprocessorCallbacks(Preprocessor ) {
+  // Enable generating macro debug info only in FullDebugInfo mode.
+  if (CodeGenOpts.getDebugInfo() < CodeGenOptions::FullDebugInfo ||
+  !getModuleDebugInfo())
+return nullptr;
+
+  return std::make_unique(*getModuleDebugInfo(), PP);
+}
+
 void CodeGenModule::addReplacement(StringRef Name, llvm::Constant *C) {
   Replacements[Name] = C;
 }
Index: lib/CodeGen/MacroPPCallbacks.h
===
--- lib/CodeGen/MacroPPCallbacks.h
+++ lib/CodeGen/MacroPPCallbacks.h
@@ -0,0 +1,82 @@
+//===--- MacroPPCallbacks.h -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+//  This file defines implementation for the macro preprocessors callbacks.
+//
+//===--===//
+
+#include "clang/Lex/PPCallbacks.h"
+#include "clang/Parse/Parser.h"
+#include 
+
+namespace llvm {
+class DIMacroFile;
+}
+namespace clang {
+
+namespace CodeGen {
+class CGDebugInfo;
+}
+class MacroPPCallbacks : public PPCallbacks {
+  /// Debug info code generator
+  CodeGen::CGDebugInfo 
+  /// Preprocessor
+  Preprocessor 
+  /// Location (used for line number) for recent included file.
+  SourceLocation LastHashLoc;
+  /// Location (used for file name) for first included file (source main).
+  SourceLocation FirstIncludeFile;
+  /// Indicates that first file inclusion occurred.
+  bool FirstInclude;
+  /// Indicates that DIMacroFile entry was created for first included file.
+  bool FirstIncludeDone;
+  /// Counts current number of command line included files, which was entered
+  /// and was not exited yet.
+  int CommandIncludeFiles;
+  /// Number of fake files that should skip that were not exited yet.
+  int SkipFiles;
+  /// Parent contains all entered files that were not exited yet according to
+  /// the inclusion order.
+  std::vector Parents;
+
+  /// Use the passed preprocessor to calculate the macro name and value from
+  /// the given macro info and identifier info.
+  static void getMacroDefinition(const IdentifierInfo , const MacroInfo ,
+ Preprocessor , raw_ostream ,
+ raw_ostream );
+
+public:
+  MacroPPCallbacks(CodeGen::CGDebugInfo , Preprocessor );
+
+  /// Callback invoked whenever a source 

Re: [PATCH] D16135: Macro Debug Info support in Clang

[Adrian Prantl via cfe-commits]

aprantl added inline comments.


Comment at: lib/CodeGen/CGDebugInfo.cpp:2099
@@ -2098,1 +2098,3 @@
 
+llvm::DIMacro *CGDebugInfo::CreateMacro(llvm::DIMacroFile *Parent, bool 
IsUndef,
+SourceLocation LineLoc, StringRef Name,

By using a bool for IsUndef, we'll end up with somewhat ugly call sites like  
  DI->CreateMacro(Parent, /*IsUndef*/ true, ...);
so we might as well just pass in the dwarf constant
  DI->CreateMacro(Parent, llvm::dwarf::DW_MACINFO_undef, ...);
directly.


http://reviews.llvm.org/D16135



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

Re: [PATCH] D16135: Macro Debug Info support in Clang

[Amjad Aboud via cfe-commits]

aaboud updated this revision to Diff 46034.
aaboud marked 6 inline comments as done.
aaboud added a comment.

Added comments explaining the implementation.


http://reviews.llvm.org/D16135

Files:
  include/clang/AST/ASTConsumer.h
  lib/AST/ASTConsumer.cpp
  lib/CodeGen/CGDebugInfo.cpp
  lib/CodeGen/CGDebugInfo.h
  lib/CodeGen/CMakeLists.txt
  lib/CodeGen/CodeGenAction.cpp
  lib/CodeGen/CodeGenModule.cpp
  lib/CodeGen/CodeGenModule.h
  lib/CodeGen/MacroPPCallbacks.cpp
  lib/CodeGen/MacroPPCallbacks.h
  lib/CodeGen/ModuleBuilder.cpp
  lib/Parse/ParseAST.cpp

Index: lib/Parse/ParseAST.cpp
===
--- lib/Parse/ParseAST.cpp
+++ lib/Parse/ParseAST.cpp
@@ -128,6 +128,12 @@
   new Parser(S.getPreprocessor(), S, SkipFunctionBodies));
   Parser  = *ParseOP.get();
 
+  std::unique_ptr Callbacks =
+  Consumer->CreatePreprocessorCallbacks(S.getPreprocessor());
+  if (Callbacks) {
+S.getPreprocessor().addPPCallbacks(std::move(Callbacks));
+  }
+
   llvm::CrashRecoveryContextCleanupRegistrar
   CleanupPrettyStack(llvm::SavePrettyStackState());
   PrettyStackTraceParserEntry CrashInfo(P);
Index: lib/CodeGen/ModuleBuilder.cpp
===
--- lib/CodeGen/ModuleBuilder.cpp
+++ lib/CodeGen/ModuleBuilder.cpp
@@ -20,6 +20,7 @@
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Frontend/CodeGenOptions.h"
+#include "clang/Lex/PPCallbacks.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/LLVMContext.h"
@@ -234,6 +235,11 @@
 void HandleDependentLibrary(llvm::StringRef Lib) override {
   Builder->AddDependentLib(Lib);
 }
+
+std::unique_ptr
+CreatePreprocessorCallbacks(Preprocessor ) override {
+  return Builder->createPreprocessorCallbacks(PP);
+}
   };
 }
 
Index: lib/CodeGen/CodeGenModule.cpp
===
--- lib/CodeGen/CodeGenModule.cpp
+++ lib/CodeGen/CodeGenModule.cpp
@@ -24,6 +24,7 @@
 #include "CodeGenPGO.h"
 #include "CodeGenTBAA.h"
 #include "CoverageMappingGen.h"
+#include "MacroPPCallbacks.h"
 #include "TargetInfo.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/CharUnits.h"
@@ -208,6 +209,16 @@
   CUDARuntime = CreateNVCUDARuntime(*this);
 }
 
+std::unique_ptr
+CodeGenModule::createPreprocessorCallbacks(Preprocessor ) {
+  // Enable generating macro debug info only in FullDebugInfo mode.
+  if (CodeGenOpts.getDebugInfo() < CodeGenOptions::FullDebugInfo ||
+  !getModuleDebugInfo())
+return nullptr;
+
+  return std::make_unique(*getModuleDebugInfo(), PP);
+}
+
 void CodeGenModule::addReplacement(StringRef Name, llvm::Constant *C) {
   Replacements[Name] = C;
 }
Index: lib/CodeGen/MacroPPCallbacks.h
===
--- lib/CodeGen/MacroPPCallbacks.h
+++ lib/CodeGen/MacroPPCallbacks.h
@@ -0,0 +1,82 @@
+//===--- MacroPPCallbacks.h -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+//  This file defines implementation for the macro preprocessors callbacks.
+//
+//===--===//
+
+#include "clang/Lex/PPCallbacks.h"
+#include "clang/Parse/Parser.h"
+#include 
+
+namespace llvm {
+class DIMacroFile;
+}
+namespace clang {
+
+namespace CodeGen {
+class CGDebugInfo;
+}
+class MacroPPCallbacks : public PPCallbacks {
+  /// Debug info code generator
+  CodeGen::CGDebugInfo 
+  /// Preprocessor
+  Preprocessor 
+  /// Location (used for line number) for recent included file.
+  SourceLocation LastHashLoc;
+  /// Location (used for file name) for first included file (source main).
+  SourceLocation FirstIncludeFile;
+  /// Indicates that first file inclusion occurred.
+  bool FirstInclude;
+  /// Indicates that DIMacroFile entry was created for first included file.
+  bool FirstIncludeDone;
+  /// Counts current number of command line included files, which was entered
+  /// and was not exited yet.
+  int CommandIncludeFiles;
+  /// Number of fake files that should skip that were not exited yet.
+  int SkipFiles;
+  /// Parent contains all entered files that were not exited yet according to
+  /// the inclusion order.
+  std::vector Parents;
+
+  /// Use the passed preprocessor to calculate the macro name and value from
+  /// the given macro info and identifier info.
+  static void getMacroDefinition(const IdentifierInfo , const MacroInfo ,
+ Preprocessor , raw_ostream ,
+ raw_ostream );
+
+public:
+  MacroPPCallbacks(CodeGen::CGDebugInfo , Preprocessor );
+
+  /// Callback 

Re: [PATCH] D16135: Macro Debug Info support in Clang

[Amjad Aboud via cfe-commits]

aaboud added inline comments.


Comment at: include/clang/AST/ASTConsumer.h:163
@@ -155,1 +162,3 @@
+  /// The caller takes ownership on the returned pointer.
+  virtual std::unique_ptr 
CreatePreprocessorCallbacks(Preprocessor );
 };

Richard,
I know that you suggested not to introduce the Preprocessor in anyway to the 
ASTConsumer.
However, as I could not understand the other way to support macros in Clang 
without applying a huge redesign, I decided to upload the code I suggested in 
the proposal.
http://lists.llvm.org/pipermail/llvm-dev/2015-November/092449.html

If you still think that this approach is not the right one, I would appreciate 
it if you can explain again how to implement the macro support differently.


http://reviews.llvm.org/D16135



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

[PATCH] D16135: Macro Debug Info support in Clang

[Amjad Aboud via cfe-commits]

aaboud created this revision.
aaboud added reviewers: dblaikie, rsmith, aprantl, probinson.
aaboud added a subscriber: cfe-commits.

Added support for FE Clang to create Debug Info entries (DIMacro and 
DIMacroFile) into generated module if "debug-info-kind" flag is set to 
"standalone".

http://reviews.llvm.org/D16135

Files:
  include/clang/AST/ASTConsumer.h
  lib/AST/ASTConsumer.cpp
  lib/CodeGen/CGDebugInfo.cpp
  lib/CodeGen/CGDebugInfo.h
  lib/CodeGen/CMakeLists.txt
  lib/CodeGen/CodeGenAction.cpp
  lib/CodeGen/CodeGenModule.cpp
  lib/CodeGen/CodeGenModule.h
  lib/CodeGen/MacroPPCallbacks.cpp
  lib/CodeGen/MacroPPCallbacks.h
  lib/CodeGen/ModuleBuilder.cpp
  lib/Parse/ParseAST.cpp

Index: lib/Parse/ParseAST.cpp
===
--- lib/Parse/ParseAST.cpp
+++ lib/Parse/ParseAST.cpp
@@ -128,6 +128,12 @@
   new Parser(S.getPreprocessor(), S, SkipFunctionBodies));
   Parser  = *ParseOP.get();
 
+  std::unique_ptr Callbacks =
+  Consumer->CreatePreprocessorCallbacks(S.getPreprocessor());
+  if (Callbacks) {
+S.getPreprocessor().addPPCallbacks(std::move(Callbacks));
+  }
+
   llvm::CrashRecoveryContextCleanupRegistrar
   CleanupPrettyStack(llvm::SavePrettyStackState());
   PrettyStackTraceParserEntry CrashInfo(P);
Index: lib/CodeGen/ModuleBuilder.cpp
===
--- lib/CodeGen/ModuleBuilder.cpp
+++ lib/CodeGen/ModuleBuilder.cpp
@@ -20,6 +20,7 @@
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Frontend/CodeGenOptions.h"
+#include "clang/Lex/PPCallbacks.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/LLVMContext.h"
@@ -234,6 +235,11 @@
 void HandleDependentLibrary(llvm::StringRef Lib) override {
   Builder->AddDependentLib(Lib);
 }
+
+std::unique_ptr
+CreatePreprocessorCallbacks(Preprocessor ) override {
+  return Builder->createPreprocessorCallbacks(PP);
+}
   };
 }
 
Index: lib/CodeGen/CodeGenModule.cpp
===
--- lib/CodeGen/CodeGenModule.cpp
+++ lib/CodeGen/CodeGenModule.cpp
@@ -24,6 +24,7 @@
 #include "CodeGenPGO.h"
 #include "CodeGenTBAA.h"
 #include "CoverageMappingGen.h"
+#include "MacroPPCallbacks.h"
 #include "TargetInfo.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/CharUnits.h"
@@ -208,6 +209,16 @@
   CUDARuntime = CreateNVCUDARuntime(*this);
 }
 
+std::unique_ptr
+CodeGenModule::createPreprocessorCallbacks(Preprocessor ) {
+  // Enable generating macro debug info only in FullDebugInfo mode.
+  if (CodeGenOpts.getDebugInfo() < CodeGenOptions::FullDebugInfo ||
+  !getModuleDebugInfo())
+return nullptr;
+
+  return std::make_unique(*getModuleDebugInfo(), PP);
+}
+
 void CodeGenModule::addReplacement(StringRef Name, llvm::Constant *C) {
   Replacements[Name] = C;
 }
Index: lib/CodeGen/MacroPPCallbacks.h
===
--- lib/CodeGen/MacroPPCallbacks.h
+++ lib/CodeGen/MacroPPCallbacks.h
@@ -0,0 +1,94 @@
+//===--- MacroPPCallbacks.h ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+//  This file defines implementation for the macro preprocessors callbacks.
+//
+//===--===//
+
+#include "clang/Lex/PPCallbacks.h"
+#include "clang/Parse/Parser.h"
+#include 
+
+namespace llvm {
+  class DIMacroFile;
+}
+namespace clang {
+
+namespace CodeGen {
+  class CGDebugInfo;
+}
+class MacroPPCallbacks : public PPCallbacks {
+  CodeGen::CGDebugInfo 
+  Preprocessor 
+  SourceLocation LastHashLoc;
+  SourceLocation FirstIncludeFile;
+  bool FirstInclude;
+  bool FirstIncludeDone;
+  int CommandIncludeFiles;
+  int SkipFiles;
+  std::vector Parents;
+
+
+  static void getMacroDefinition(const IdentifierInfo , const MacroInfo ,
+ Preprocessor , raw_ostream ,
+ raw_ostream );
+
+public:
+  MacroPPCallbacks(CodeGen::CGDebugInfo& DI, Preprocessor );
+
+  /// \brief Callback invoked whenever a source file is entered or exited.
+  ///
+  /// \param Loc Indicates the new location.
+  /// \param PrevFID the file that was exited if \p Reason is ExitFile.
+  virtual void FileChanged(SourceLocation Loc, FileChangeReason Reason,
+   SrcMgr::CharacteristicKind FileType,
+   FileID PrevFID = FileID());
+
+  /// \brief Callback invoked whenever a source file is skipped as the result
+  /// of header guard optimization.
+  ///
+  /// \param SkippedFile The file that is skipped instead of entering \#include
+  ///
+  ///