[PATCH] D38061: Set AnonymousTagLocations false for ASTContext if column info is expected not to be used

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

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)


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D38061/new/

https://reviews.llvm.org/D38061



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

[PATCH] D38061: Set AnonymousTagLocations false for ASTContext if column info is expected not to be used

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

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)


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D38061/new/

https://reviews.llvm.org/D38061



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

[PATCH] D38061: Set AnonymousTagLocations false for ASTContext if column info is expected not to be used

[Wenlei He via Phabricator via cfe-commits]

wenlei added a comment.

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.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D38061/new/

https://reviews.llvm.org/D38061



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

[PATCH] D38061: Set AnonymousTagLocations false for ASTContext if column info is expected not to be used

[Taewook Oh via Phabricator via cfe-commits]

twoh added a comment.

Friendly ping for comments. Thanks!


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D38061/new/

https://reviews.llvm.org/D38061



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

[PATCH] D38061: Set AnonymousTagLocations false for ASTContext if column info is expected not to be used

[Taewook Oh via Phabricator via cfe-commits]

twoh added a subscriber: wenlei.
twoh added a comment.
Herald added a subscriber: dexonsmith.
Herald added a project: clang.

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?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D38061/new/

https://reviews.llvm.org/D38061



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

[PATCH] D38061: Set AnonymousTagLocations false for ASTContext if column info is expected not to be used

[Taewook Oh via Phabricator via cfe-commits]

twoh added a comment.

@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.


Repository:
  rC Clang

https://reviews.llvm.org/D38061



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

[PATCH] D38061: Set AnonymousTagLocations false for ASTContext if column info is expected not to be used

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith requested changes to this revision.
rsmith added inline comments.
This revision now requires changes to proceed.



Comment at: lib/Frontend/CompilerInstance.cpp:491-494
+  PrintingPolicy Policy = Context->getPrintingPolicy();
+  if (!getCodeGenOpts().DebugColumnInfo)
+Policy.AnonymousTagLocations = false;
+  Context->setPrintingPolicy(Policy);

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


Repository:
  rC Clang

https://reviews.llvm.org/D38061



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

[PATCH] D38061: Set AnonymousTagLocations false for ASTContext if column info is expected not to be used

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In https://reviews.llvm.org/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.


Repository:
  rC Clang

https://reviews.llvm.org/D38061



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

[PATCH] D38061: Set AnonymousTagLocations false for ASTContext if column info is expected not to be used

[Taewook Oh via Phabricator via cfe-commits]

twoh added a comment.

@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!


Repository:
  rC Clang

https://reviews.llvm.org/D38061



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

[PATCH] D38061: Set AnonymousTagLocations false for ASTContext if column info is expected not to be used

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In https://reviews.llvm.org/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 /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 >" - 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)


Repository:
  rC Clang

https://reviews.llvm.org/D38061



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

[PATCH] D38061: Set AnonymousTagLocations false for ASTContext if column info is expected not to be used

[Taewook Oh via Phabricator via cfe-commits]

twoh added a comment.

@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",
 linkageName: "_Z3fooIN1XUt_EEiT_" ...`, which will eventually be included in 
.debug_info section.


Repository:
  rC Clang

https://reviews.llvm.org/D38061



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

Re: [PATCH] D38061: Set AnonymousTagLocations false for ASTContext if column info is expected not to be used

[David Blaikie via cfe-commits]

On Fri, Oct 19, 2018 at 3:56 PM Adrian Prantl via Phabricator via
llvm-commits  wrote:

> aprantl added a comment.
>
> 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.
>

It was (I think) duplicate calls (rather than types) - so that inlining
(which previously didn't produce a unique'd inlinedAt location (because we
didn't have the "unique" metadata feature)) didn't collapse the
instructions from the two inlined calls into one.


>
>
> Repository:
>   rC Clang
>
> https://reviews.llvm.org/D38061
>
>
>
> ___
> llvm-commits mailing list
> llvm-comm...@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38061: Set AnonymousTagLocations false for ASTContext if column info is expected not to be used

[Adrian Prantl via Phabricator via cfe-commits]

aprantl added a comment.

RTTI?


Repository:
  rC Clang

https://reviews.llvm.org/D38061



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

[PATCH] D38061: Set AnonymousTagLocations false for ASTContext if column info is expected not to be used

[Adrian Prantl via Phabricator via cfe-commits]

aprantl added a comment.

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


Repository:
  rC Clang

https://reviews.llvm.org/D38061



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

[PATCH] D38061: Set AnonymousTagLocations false for ASTContext if column info is expected not to be used

[Adrian Prantl via Phabricator via cfe-commits]

aprantl added a comment.

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.


Repository:
  rC Clang

https://reviews.llvm.org/D38061



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

[PATCH] D38061: Set AnonymousTagLocations false for ASTContext if column info is expected not to be used

[Eric Christopher via Phabricator via cfe-commits]

echristo added a reviewer: aprantl.
echristo added a comment.

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


Repository:
  rC Clang

https://reviews.llvm.org/D38061



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

[PATCH] D38061: Set AnonymousTagLocations false for ASTContext if column info is expected not to be used

[Taewook Oh via Phabricator via cfe-commits]

twoh added a comment.

ping!


Repository:
  rC Clang

https://reviews.llvm.org/D38061



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

[PATCH] D38061: Set AnonymousTagLocations false for ASTContext if column info is expected not to be used

[Taewook Oh via Phabricator via cfe-commits]

twoh updated this revision to Diff 164564.
twoh edited the summary of this revision.
twoh added a comment.
Herald added a subscriber: cfe-commits.

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.


Repository:
  rC Clang

https://reviews.llvm.org/D38061

Files:
  lib/AST/Expr.cpp
  lib/CodeGen/CGDebugInfo.cpp
  lib/Frontend/CompilerInstance.cpp
  test/CXX/expr/expr.prim/expr.prim.lambda/default-arguments.cpp
  test/CodeGenCXX/debug-info-anonymous.cpp
  test/Sema/assign.c
  test/Sema/enum.c
  test/Sema/switch.c
  test/SemaCXX/condition.cpp
  test/SemaCXX/enum.cpp
  test/SemaCXX/lambda-expressions.cpp
  test/SemaCXX/pass-object-size.cpp
  test/SemaCXX/warn-sign-conversion.cpp

Index: test/SemaCXX/warn-sign-conversion.cpp
===
--- test/SemaCXX/warn-sign-conversion.cpp
+++ test/SemaCXX/warn-sign-conversion.cpp
@@ -1,4 +1,4 @@
-// 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.
Index: test/SemaCXX/pass-object-size.cpp
===
--- test/SemaCXX/pass-object-size.cpp
+++ test/SemaCXX/pass-object-size.cpp
@@ -1,4 +1,4 @@
-// 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;
Index: test/SemaCXX/lambda-expressions.cpp
===
--- test/SemaCXX/lambda-expressions.cpp
+++ test/SemaCXX/lambda-expressions.cpp
@@ -1,4 +1,4 @@
-// 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; };
 
Index: test/SemaCXX/enum.cpp
===
--- test/SemaCXX/enum.cpp
+++ test/SemaCXX/enum.cpp
@@ -1,5 +1,5 @@
-// 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
Index: test/SemaCXX/condition.cpp
===
--- test/SemaCXX/condition.cpp
+++ test/SemaCXX/condition.cpp
@@ -1,4 +1,4 @@
-// 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;
Index: test/Sema/switch.c
===
--- test/Sema/switch.c
+++ test/Sema/switch.c
@@ -1,8 +1,8 @@
-// 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) {
@@ -22,7 +22,7 @@
   }
 }
 
-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}} \
@@ -45,33 +45,33 @@
   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';
@@ -187,7 +187,7 @@
 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