Set AnonymousTagLocations false for ASTContext if column info is expected not to be used

[twoh]

Summary

Currently, location (line, column number) information of anonymous types are printed even when the column location infomation is expected to be turned off. These information are even included in the final binary as strings, and make clang to generate different output when preprocessed source file is given as an input instead of the original source code (because of the macro expansion). This inhibits the use of clang with a build system that supports both distcc and caching, as distcc feeds preprocessed source file to remote build machines.

This patch turns off location information of anonymous types if -debug-column-info is not provided. I renamed -dwarf-column-info to -debug-column-info as it is not only for dwarf now (and the corresponding codegen option name was DebugColumnInfo, as shown in lib/Frontend/CompilerInvocation.cpp:521).

[twoh]

Ping, Thanks!

[twoh]

Ping. Thanks!

[echristo]

Meta comment: please split the rename out. I don't mind the rename, but it makes the change much harder to review.

Needs a testcase for the actual behavior you're trying to ensure here.

Thanks!

[twoh]

Addressing comments from @echristo. Reverted option name change, and added a test case. Sorry I haven't work on this code for a while so it took time to invent a test case.

[twoh]

ping!

[echristo]

I think Adrian has looked at this more recently than I have. Adding him here.

[aprantl]

I have a vague recollection that this column info hack was added to disambiguate two types defined on the same line (which is something that happened more often than one would think because of macro expansion). Did you do the git archeology to ensure that the original reason for doing it this way has been resolved? FWIW, I'm fine with doing this change for the Darwin platforms because column info is turned on by default, so this shouldn't affect us.

[aprantl]

Oh wait, this patch is just for dumping the ASTs? Can you elaborate why this makes it into a binary then?

[aprantl]

RTTI?

[twoh]

@aprantl It is a debug info. If you compile test/CodeGenCXX/debug-info-anonymous.cpp file with clang -g2 -emit-llvm -S , you will find debug metadata something like distinct !DISubprogram(name: "foo<X::(anonymous enum at /home/twoh/llvms/llvm/tools/clang/test/CodeGenCXX/debug-info-anonymous.cpp:9:3)>", linkageName: "_Z3fooIN1XUt_EEiT_" ..., which will eventually be included in .debug_info section.

[dblaikie]

In D38061#1271021, @twoh wrote:

@aprantl It is a debug info. If you compile test/CodeGenCXX/debug-info-anonymous.cpp file with clang -g2 -emit-llvm -S , you will find debug metadata something like distinct !DISubprogram(name: "foo<X::(anonymous enum at /home/twoh/llvms/llvm/tools/clang/test/CodeGenCXX/debug-info-anonymous.cpp:9:3)>", linkageName: "_Z3fooIN1XUt_EEiT_" ..., which will eventually be included in .debug_info section.

For comparison, GCC names these "foo<X::<anonymous enum> >" - this is somewhat of a compatibility problem, since the template function definition's names won't match between GCC and Clang, but I guess this is by far not the only instance of that. GCC keeps that naming scheme even when it's clearly ambiguous (ie: it's not just making a short name when there's no other X::anonymous enum, it does it even when there's two of them, etc)

[twoh]

@dblaikie I see. The problem we're experiencing is that with Clang's naming scheme we end up having different function name between the original source and the preprocessed source (as macro expansion changes the column number). This doesn't work well for me if I want to use distcc on top of our caching system, as the caching scheme expects the output to be same as far as the original source has not been changed (regardless of it's compiled directly or first preprocessed then compiled), but the distcc preprocesses the source locally then sends it to the remote build machine (and we do not turn distcc on for all workflow). I wonder if you have any suggestion to resolve this issue? Thanks!

[dblaikie]

In D38061#1275845, @twoh wrote:

@dblaikie I see. The problem we're experiencing is that with Clang's naming scheme we end up having different function name between the original source and the preprocessed source (as macro expansion changes the column number).

I imagine that's not achievable, though - as soon as you have a multi-line macro at least, it seems like it'd be difficult to preserve (I guess you could put loc directives between every line in the macro to ensure every line was attributed to the line the macro was written on, maybe?)

This doesn't work well for me if I want to use distcc on top of our caching system, as the caching scheme expects the output to be same as far as the original source has not been changed (regardless of it's compiled directly or first preprocessed then compiled), but the distcc preprocesses the source locally then sends it to the remote build machine (and we do not turn distcc on for all workflow). I wonder if you have any suggestion to resolve this issue? Thanks!

Best thing to do, I think, if possible, is teach the distributed build system not to fully preprocess but to use something like Clang's -frewrite-includes - this handles includes, but leaves macro definitions and uses in-place. This would preserve Clang's warning semantics (where macros can help inform Clang's diagnostic choices, improving diagnostic quality (lowering false positives, etc)) and other things like debug info.

[rsmith]

lib/Frontend/CompilerInstance.cpp

491–494

It's not reasonable for a debug info flag to affect the printing policy used throughout the compiler.

[twoh]

@rsmith @dblaikie Thank you for the comments! It seems that this is not the appropriate way to handle the issue. I'll find different way to resolve the problem.

[twoh]

Hello @rsmith, @wenlei and I took another look at this, and we couldn't find any use of AnonymousTagLocations outside of debug info. If that's actually the case, wouldn't it make sense to have DebugColumnInfo to control the field even if AnonymousTagLocations the part of generic printing policy?

[twoh]

Friendly ping for comments. Thanks!

[dblaikie]

Seems like the right thing would be for the DWARF code that wants a rendered type name to pass its own printing policy, rather than changing some relatively global one.

(though also I have my doubts about the whole approach - macro expansion can change the line number as well as the column number, so only suppressing column number would be insufficient - and this also reduces the usability for users (because the file/line/column of an anonymous type is a useful debugging aid). Seems to me like this should be opt-in if it's supported at all - though ideally build systems would use -frewrite-includes rather than full preprocessing, and then macros and lines/columns would be preserved, I think)

[wenlei]

In D38061#1484486, @dblaikie wrote:

Seems like the right thing would be for the DWARF code that wants a rendered type name to pass its own printing policy, rather than changing some relatively global one.

(though also I have my doubts about the whole approach - macro expansion can change the line number as well as the column number, so only suppressing column number would be insufficient - and this also reduces the usability for users (because the file/line/column of an anonymous type is a useful debugging aid). Seems to me like this should be opt-in if it's supported at all - though ideally build systems would use -frewrite-includes rather than full preprocessing, and then macros and lines/columns would be preserved, I think)

Line number is ok because preprocessor also inserts #linemarker, and later on these markers are used to recover the original line number before macro expansion. Column is problematic, as there's nothing like a column marker (if possible at all) so there's no way to see through the column change from macro expansion. We tried -frewrite-includes too, but it has its own issues - without macro expansion, it bloats the file size that needs to send to distcc remote machine significantly.

[dblaikie]

In D38061#1484568, @wenlei wrote:

In D38061#1484486, @dblaikie wrote:

Seems like the right thing would be for the DWARF code that wants a rendered type name to pass its own printing policy, rather than changing some relatively global one.

(though also I have my doubts about the whole approach - macro expansion can change the line number as well as the column number, so only suppressing column number would be insufficient - and this also reduces the usability for users (because the file/line/column of an anonymous type is a useful debugging aid). Seems to me like this should be opt-in if it's supported at all - though ideally build systems would use -frewrite-includes rather than full preprocessing, and then macros and lines/columns would be preserved, I think)

Line number is ok because preprocessor also inserts #linemarker, and later on these markers are used to recover the original line number before macro expansion. Column is problematic, as there's nothing like a column marker (if possible at all) so there's no way to see through the column change from macro expansion. We tried -frewrite-includes too, but it has its own issues - without macro expansion, it bloats the file size that needs to send to distcc remote machine significantly.

Do you have some data on that growth/size comparison & its overall performance impact on the build? I'd be curious.

But, yeah, fair point about the line markers - I'd thought a multiline macro would break that, but seems not (it gets stamped out by the preprocessor all on one line anyway).

Still - figure this should be opt-in and done at the debug info level, not by changing a global printing policy. (& I'd still suggest using -frewrite-includes because Clang changes its diagnostic behavior based on the presence of macros to reduce diagnostic false positives, so you'll be degrading the usability in other ways by compiling fully preprocessed code)

lib/AST/Expr.cpp

if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(CurrentDecl)) {
 
     if (const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(FD)) {
       if (MD->isVirtual() && IT != PrettyFunctionNoVirtual)
         Out << "virtual ";
       if (MD->isStatic())
         Out << "static ";
     }
 
-    PrintingPolicy Policy(Context.getLangOpts());
+    PrintingPolicy Policy(Context.getPrintingPolicy());
     std::string Proto;
     llvm::raw_string_ostream POut(Proto);
 
     const FunctionDecl *Decl = FD;
     if (const FunctionDecl* Pattern = FD->getTemplateInstantiationPattern())
       Decl = Pattern;
     const FunctionType *AFT = Decl->getType()->getAs<FunctionType>();
     const FunctionProtoType *FT = nullptr;

lib/CodeGen/CGDebugInfo.cpp

PrintingPolicy CGDebugInfo::getPrintingPolicy() const {
 
   // If we're emitting codeview, it's important to try to match MSVC's naming so
   // that visualizers written for MSVC will trigger for our class names. In
   // particular, we can't have spaces between arguments of standard templates
   // like basic_string and vector.
   if (CGM.getCodeGenOpts().EmitCodeView)
     PP.MSVCFormatting = true;
 
+  if (!CGM.getCodeGenOpts().DebugColumnInfo)
+    PP.AnonymousTagLocations = false;
+
   return PP;
 }
 
 StringRef CGDebugInfo::getFunctionName(const FunctionDecl *FD) {
   assert(FD && "Invalid FunctionDecl!");
   IdentifierInfo *FII = FD->getIdentifier();
   FunctionTemplateSpecializationInfo *Info =
       FD->getTemplateSpecializationInfo();

lib/Frontend/CompilerInstance.cpp

 // ASTContext
 
 void CompilerInstance::createASTContext() {
   Preprocessor &PP = getPreprocessor();
   auto *Context = new ASTContext(getLangOpts(), PP.getSourceManager(),
                                  PP.getIdentifierTable(), PP.getSelectorTable(),
                                  PP.getBuiltinInfo());
   Context->InitBuiltinTypes(getTarget(), getAuxTarget());
+  PrintingPolicy Policy = Context->getPrintingPolicy();
+  if (!getCodeGenOpts().DebugColumnInfo)
+    Policy.AnonymousTagLocations = false;
+  Context->setPrintingPolicy(Policy);
   setASTContext(Context);
 }
 
 // ExternalASTSource
 
 void CompilerInstance::createPCHExternalASTSource(
     StringRef Path, bool DisablePCHValidation, bool AllowPCHWithCompilerErrors,
     void *DeserializationListener, bool OwnDeserializationListener) {

test/CXX/expr/expr.prim/expr.prim.lambda/default-arguments.cpp

-// RUN: %clang_cc1 -fsyntax-only -std=c++11 %s -verify
+// RUN: %clang_cc1 -dwarf-column-info -fsyntax-only -std=c++11 %s -verify
 
 void defargs() {
   auto l1 = [](int i, int j = 17, int k = 18) { return i + j + k; };
   int i1 = l1(1);
   int i2 = l1(1, 2);
   int i3 = l1(1, 2, 3);
 }
 

test/CodeGenCXX/debug-info-anonymous.cpp

+template <class T> int foo(const T v) {
+  return v;
+}
+
+int foo(int v) { return foo<int>(v); }
+
+class X {
+public:
+  enum { A };
+  int x() { return foo(A); }
+};
+
+int test() {
+  X x;
+  return x.x();
+}
+
+// RUN: %clang_cc1 -emit-llvm -debug-info-kind=standalone %s -o - | FileCheck --check-prefix=NOLOC %s
+// RUN: %clang_cc1 -emit-llvm -dwarf-column-info -debug-info-kind=standalone %s -o - | FileCheck --check-prefix=LOC %s
+
+// NOLOC: X::(anonymous)
+// LOC: X::(anonymous enum at {{.*}}:9:3)

test/Sema/assign.c

-// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -dwarf-column-info -fsyntax-only -verify %s
 
 void *test1(void) { return 0; }
 
 void test2 (const struct {int a;} *x) {
   // expected-note@-1 {{variable 'x' declared const here}}
 
   x->a = 10;
   // expected-error-re@-1 {{cannot assign to variable 'x' with const-qualified type 'const struct (anonymous struct at {{.*}}assign.c:5:19) *'}}

test/Sema/enum.c

-// RUN: %clang_cc1 -triple %itanium_abi_triple %s -fsyntax-only -verify -pedantic
-enum e {A, 
+// RUN: %clang_cc1 -triple %itanium_abi_triple %s -dwarf-column-info -fsyntax-only -verify -pedantic
+enum e {A,
         B = 42LL << 32,        // expected-warning {{ISO C restricts enumerator values to range of 'int'}}
       C = -4, D = 12456 };
 
 enum f { a = -2147483648, b = 2147483647 }; // ok.
 
 enum g {  // too negative
    c = -2147483649,         // expected-warning {{ISO C restricts enumerator values to range of 'int'}}
    d = 2147483647 };
 enum h { e = -2147483648, // too pos
    f = 2147483648,           // expected-warning {{ISO C restricts enumerator values to range of 'int'}}
   i = 0xFFFF0000 // expected-warning {{too large}}
-}; 
+};
 
 // minll maxull
 enum x                      // expected-warning {{enumeration values exceed range of largest integer}}
 { y = -9223372036854775807LL-1,  // expected-warning {{ISO C restricts enumerator values to range of 'int'}}
 z = 9223372036854775808ULL };    // expected-warning {{ISO C restricts enumerator values to range of 'int'}}
 
 int test() {
   return sizeof(enum e) ;

test/Sema/switch.c

-// RUN: %clang_cc1 -fsyntax-only -verify -Wswitch-enum -Wcovered-switch-default -triple x86_64-linux-gnu %s
-void f (int z) { 
-  while (z) { 
+// RUN: %clang_cc1 -dwarf-column-info -fsyntax-only -verify -Wswitch-enum -Wcovered-switch-default -triple x86_64-linux-gnu %s
+void f (int z) {
+  while (z) {
     default: z--;            // expected-error {{statement not in switch}}
-  } 
+  }
 }
 
 void foo(int X) {
   switch (X) {
   case 42: ;                 // expected-note {{previous case}}
   case 5000000000LL:         // expected-warning {{overflow}}
   case 42:                   // expected-error {{duplicate case value '42'}}
    ;
 
   case 100 ... 99: ;         // expected-warning {{empty case range}}
 
   case 43: ;                 // expected-note {{previous case}}
   case 43 ... 45:  ;         // expected-error {{duplicate case value}}
 
   case 100 ... 20000:;       // expected-note {{previous case}}
   case 15000 ... 40000000:;  // expected-error {{duplicate case value}}
   }
 }
 
-void test3(void) { 
+void test3(void) {
   // empty switch;
   switch (0); // expected-warning {{no case matching constant switch condition '0'}} \
               // expected-warning {{switch statement has empty body}} \
               // expected-note{{put the semicolon on a separate line to silence this warning}}
 }
 
 extern int g();
 
 void test4()
 {
   int cond;
   switch (cond) {
   case 0 && g():
   case 1 || g():
     break;
   }
 
   switch(cond)  {
   case g(): // expected-error {{expression is not an integer constant expression}}
   case 0 ... g(): // expected-error {{expression is not an integer constant expression}}
     break;
   }
-  
   switch (cond) {
   case 0 && g() ... 1 || g():
     break;
   }
-  
   switch (cond) {
   case g() // expected-error {{expression is not an integer constant expression}}
       && 0:
     break;
   }
-  
   switch (cond) {
   case 0 ...
       g() // expected-error {{expression is not an integer constant expression}}
       || 1:
     break;
   }
 }
 
-void test5(int z) { 
+void test5(int z) {
   switch(z) {
     default:  // expected-note {{previous case defined here}}
     default:  // expected-error {{multiple default labels in one switch}}
       break;
   }
-} 
+}
 
 void test6() {
   char ch = 'a';
   switch(ch) {
     case 1234:  // expected-warning {{overflow converting case value}}
       break;
   }
 }
enum {
     C = 2,
     B = 4,
     D = 12
   } a;
   switch(a) {
     case 0 ...  //expected-warning{{case value not in enumerated type 'enum (anonymous enum}}
       1:  //expected-warning{{case value not in enumerated type 'enum (anonymous enum}}
     case 2 ... 4:
-    case 5 ...  //expected-warning{{case value not in enumerated type 'enum (anonymous enum}} 
+    case 5 ...  //expected-warning{{case value not in enumerated type 'enum (anonymous enum}}
         9:  //expected-warning{{case value not in enumerated type 'enum (anonymous enum}}
     case 10 ... 12:
     case 13 ...  //expected-warning{{case value not in enumerated type 'enum (anonymous enum}}
               16: //expected-warning{{case value not in enumerated type 'enum (anonymous enum}}
       break;
   }
 }
 
 void test11() {
   enum {
     A = -1,
     B,
     C
   } a;
   switch(a) { //expected-warning{{enumeration value 'A' not handled in switch}}
     case B:
     case C:
       break;
   }
 
   switch(a) { //expected-warning{{enumeration value 'A' not explicitly handled in switch}}
     case B:
     case C:
       break;
-      
     default:
       break;
   }
 }
 
 void test12() {
   enum {
     A = -1,
int test18() {
   default: return 2; // expected-warning {{default label in switch which covers all enumeration values}}
   }
 }
 
 // rdar://110822110
 typedef enum {
         kOne = 1,
 } Ints;
-        
 void rdar110822110(Ints i)
 {
         switch (i) {
                 case kOne:
                         break;
-                case 2:   // expected-warning {{case value not in enumerated type 'Ints'}}          
+                case 2:   // expected-warning {{case value not in enumerated type 'Ints'}}
                         break;
                 default:  // expected-warning {{default label in switch which covers all enumeration values}}
                         break;
                 }
 }
 
 // PR9243
 #define TEST19MACRO 5

test/SemaCXX/condition.cpp

-// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 %s 
+// RUN: %clang_cc1 -dwarf-column-info -fsyntax-only -verify -std=c++11 %s
 
 void test() {
   int x;
   if (x) ++x;
   if (int x=0) ++x;
 
   typedef int arr[10];
   while (arr x={0}) ; // expected-error {{an array type is not allowed here}}

test/SemaCXX/enum.cpp

-// RUN: %clang_cc1 -fsyntax-only -pedantic -std=c++98 -verify -triple x86_64-apple-darwin %s
-// RUN: %clang_cc1 -fsyntax-only -pedantic -std=c++11 -verify -triple x86_64-apple-darwin %s
+// RUN: %clang_cc1 -dwarf-column-info -fsyntax-only -pedantic -std=c++98 -verify -triple x86_64-apple-darwin %s
+// RUN: %clang_cc1 -dwarf-column-info -fsyntax-only -pedantic -std=c++11 -verify -triple x86_64-apple-darwin %s
 enum E { // expected-note{{previous definition is here}}
   Val1,
   Val2
 };
 
 enum E; // expected-warning{{redeclaration of already-defined enum 'E' is a GNU extension}}
 
 int& enumerator_type(int);

test/SemaCXX/lambda-expressions.cpp

-// RUN: %clang_cc1 -std=c++14 -Wno-unused-value -fsyntax-only -verify -fblocks %s
+// RUN: %clang_cc1 -std=c++14 -Wno-unused-value -dwarf-column-info -fsyntax-only -verify -fblocks %s
 
 namespace std { class type_info; };
 
 namespace ExplicitCapture {
   class C {
     int Member;
 
     static void Overload(int);

test/SemaCXX/pass-object-size.cpp

-// RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++11
+// RUN: %clang_cc1 -dwarf-column-info -fsyntax-only -verify %s -std=c++11
 
 namespace simple {
 int Foo(void *const p __attribute__((pass_object_size(0))));
 
 int OvlFoo(void *const p __attribute__((pass_object_size(0))));
 int OvlFoo(void *const p, int);
 
 struct Statics {

test/SemaCXX/warn-sign-conversion.cpp

-// RUN: %clang_cc1 -triple %itanium_abi_triple -verify -fsyntax-only -Wsign-conversion %s
+// RUN: %clang_cc1 -triple %itanium_abi_triple -dwarf-column-info -verify -fsyntax-only -Wsign-conversion %s
 
 // NOTE: When a 'enumeral mismatch' warning is implemented then expect several
 // of the following cases to be impacted.
 
 // namespace for anonymous enums tests
 namespace test1 {
   enum { A };
   enum { B = -1 };