[PATCH] D46685: [CodeGen] Disable structor optimizations at -O0

[Pavel Labath via Phabricator via cfe-commits]

labath created this revision.
labath added reviewers: rjmccall, aprantl.

Merging of complete and base structors can degrade debug quality as it
will leave the debugger unable to locate the full object structor. This
is apparent when evaluating an expression in the debugger which requires
constructing an object of class which has had this optimization applied
to it.  When compiling the expression, we pretend that the class and
its methods have been defined in another compilation unit, so the
expression compiler assumes the structor definition must be available,
whereas in reality the complete structor may have been omitted and all
it's uses replaced by the base structor.

This improves debug quality on non-darwin platforms (darwin does not
have -mconstructor-aliases on by default, so it is spared these
problems) and enable us to remove some workarounds from LLDB which attempt to
mitigate this issue.

This is tested by strenghtening the check in the existing
ctor-dtor-alias test. In the other aliasing tests I add -O1 to compiler
options to make sure the aliasing kicks in.

PS: Of the three ways we can do structor optimizations, only the RAUW
actually causes problems as it makes the symbol completely disappear, so
technically it should be sufficient to weaken RAUW to one of the other two
for -O0, but disabling optimizations completely looked like a more
principled solution.


Repository:
  rC Clang

https://reviews.llvm.org/D46685

Files:
  lib/CodeGen/ItaniumCXXABI.cpp
  test/CodeGenCXX/constructor-alias.cpp
  test/CodeGenCXX/ctor-dtor-alias.cpp
  test/CodeGenCXX/dllexport-alias.cpp
  test/CodeGenCXX/virtual-bases.cpp


Index: test/CodeGenCXX/virtual-bases.cpp
===
--- test/CodeGenCXX/virtual-bases.cpp
+++ test/CodeGenCXX/virtual-bases.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -emit-llvm %s -o - -triple=x86_64-apple-darwin10 
-mconstructor-aliases | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm %s -o - -triple=x86_64-apple-darwin10 
-mconstructor-aliases -O1 -disable-llvm-passes | FileCheck %s
 
 struct A { 
   A();
Index: test/CodeGenCXX/dllexport-alias.cpp
===
--- test/CodeGenCXX/dllexport-alias.cpp
+++ test/CodeGenCXX/dllexport-alias.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -triple x86_64-windows-gnu -mconstructor-aliases %s -S 
-emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-windows-gnu -mconstructor-aliases %s -S 
-emit-llvm -O1 -disable-llvm-passes -o - | FileCheck %s
 
 // This test assumes that the C1 constructor will be aliased to the C2
 // constructor, and the D1 destructor to the D2. It then checks that the 
aliases
Index: test/CodeGenCXX/ctor-dtor-alias.cpp
===
--- test/CodeGenCXX/ctor-dtor-alias.cpp
+++ test/CodeGenCXX/ctor-dtor-alias.cpp
@@ -77,11 +77,12 @@
   // CHECK1: call i32 @__cxa_atexit{{.*}}_ZN5test41AD2Ev
   // CHECK1: define linkonce_odr void @_ZN5test41AD2Ev({{.*}} comdat align
 
-  // test that we don't do this optimization at -O0 so that the debugger can
-  // see both destructors.
+  // test that we don't do this optimization at -O0 and call the complete
+  // destructor for B instead. This enables the debugger to see both
+  // destructors.
   // NOOPT: define internal void @__cxx_global_var_init.2()
-  // NOOPT: call i32 @__cxa_atexit{{.*}}@_ZN5test41BD2Ev
-  // NOOPT: define linkonce_odr void @_ZN5test41BD2Ev({{.*}} comdat align
+  // NOOPT: call i32 @__cxa_atexit{{.*}}@_ZN5test41BD1Ev
+  // NOOPT: define linkonce_odr void @_ZN5test41BD1Ev({{.*}} comdat align
   struct A {
 virtual ~A() {}
   };
Index: test/CodeGenCXX/constructor-alias.cpp
===
--- test/CodeGenCXX/constructor-alias.cpp
+++ test/CodeGenCXX/constructor-alias.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -emit-llvm -triple mipsel--linux-gnu -mconstructor-aliases 
-o - %s | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm -triple mipsel--linux-gnu -mconstructor-aliases 
-O1 -disable-llvm-passes -o - %s | FileCheck %s
 
 // The target attribute code used to get confused with aliases. Make sure
 // we don't crash when an alias is used.
Index: lib/CodeGen/ItaniumCXXABI.cpp
===
--- lib/CodeGen/ItaniumCXXABI.cpp
+++ lib/CodeGen/ItaniumCXXABI.cpp
@@ -3616,6 +3616,11 @@
   if (MD->getParent()->getNumVBases())
 return StructorCodegen::Emit;
 
+  // Omitting the complete structor can degrade debug quality as the debugger
+  // cannot locate the complete structor symbol anymore.
+  if (CGM.getCodeGenOpts().OptimizationLevel == 0)
+return StructorCodegen::Emit;
+
   GlobalDecl AliasDecl;
   if (const auto *DD = dyn_cast(MD)) {
 AliasDecl = GlobalDecl(DD, Dtor_Complete);


Index: test/CodeGenCXX/virtual-bases.cpp
===
--- test/CodeGe

[PATCH] D46685: [CodeGen] Disable structor optimizations at -O0

[Pavel Labath via Phabricator via cfe-commits]

labath updated this revision to Diff 146309.
labath added a comment.

Yes we can do that. Only the RAUW optimization is nasty, I've checked that the
debugger is able to use the alias with no problem.

Updated the patch the reflect that.


Repository:
  rC Clang

https://reviews.llvm.org/D46685

Files:
  lib/CodeGen/ItaniumCXXABI.cpp
  test/CodeGenCXX/ctor-dtor-alias.cpp
  test/CodeGenCXX/float16-declarations.cpp


Index: test/CodeGenCXX/float16-declarations.cpp
===
--- test/CodeGenCXX/float16-declarations.cpp
+++ test/CodeGenCXX/float16-declarations.cpp
@@ -103,7 +103,7 @@
 
   C1 c1(f1l);
 // CHECK-DAG:  [[F1L:%[a-z0-9]+]] = load half, half* %{{.*}}, align 2
-// CHECK-DAG:  call void @_ZN2C1C2EDF16_(%class.C1* %{{.*}}, half %{{.*}})
+// CHECK-DAG:  call void @_ZN2C1C1EDF16_(%class.C1* %{{.*}}, half %{{.*}})
 
   S1<_Float16> s1 = { 132.f16 };
 // CHECK-DAG: @_ZZ4mainE2s1 = private unnamed_addr constant %struct.S1 { half 
0xH5820 }, align 2
Index: test/CodeGenCXX/ctor-dtor-alias.cpp
===
--- test/CodeGenCXX/ctor-dtor-alias.cpp
+++ test/CodeGenCXX/ctor-dtor-alias.cpp
@@ -77,11 +77,12 @@
   // CHECK1: call i32 @__cxa_atexit{{.*}}_ZN5test41AD2Ev
   // CHECK1: define linkonce_odr void @_ZN5test41AD2Ev({{.*}} comdat align
 
-  // test that we don't do this optimization at -O0 so that the debugger can
-  // see both destructors.
+  // test that we don't do this optimization at -O0 and call the complete
+  // destructor for B instead. This enables the debugger to see both
+  // destructors.
+  // NOOPT: @_ZN5test41BD1Ev = linkonce_odr alias void {{.*}} @_ZN5test41BD2Ev
   // NOOPT: define internal void @__cxx_global_var_init.2()
-  // NOOPT: call i32 @__cxa_atexit{{.*}}@_ZN5test41BD2Ev
-  // NOOPT: define linkonce_odr void @_ZN5test41BD2Ev({{.*}} comdat align
+  // NOOPT: call i32 @__cxa_atexit{{.*}}@_ZN5test41BD1Ev
   struct A {
 virtual ~A() {}
   };
Index: lib/CodeGen/ItaniumCXXABI.cpp
===
--- lib/CodeGen/ItaniumCXXABI.cpp
+++ lib/CodeGen/ItaniumCXXABI.cpp
@@ -3625,12 +3625,16 @@
   }
   llvm::GlobalValue::LinkageTypes Linkage = CGM.getFunctionLinkage(AliasDecl);
 
-  if (llvm::GlobalValue::isDiscardableIfUnused(Linkage))
-return StructorCodegen::RAUW;
-
-  // FIXME: Should we allow available_externally aliases?
-  if (!llvm::GlobalAlias::isValidLinkage(Linkage))
-return StructorCodegen::RAUW;
+  // Only use RAUW in optimized code, as it makes the complete structor symbol
+  // disappear completely, which degrades debugging experience.
+  if (CGM.getCodeGenOpts().OptimizationLevel > 0) {
+if (llvm::GlobalValue::isDiscardableIfUnused(Linkage))
+  return StructorCodegen::RAUW;
+
+// FIXME: Should we allow available_externally aliases?
+if (!llvm::GlobalAlias::isValidLinkage(Linkage))
+  return StructorCodegen::RAUW;
+  }
 
   if (llvm::GlobalValue::isWeakForLinker(Linkage)) {
 // Only ELF and wasm support COMDATs with arbitrary names (C5/D5).


Index: test/CodeGenCXX/float16-declarations.cpp
===
--- test/CodeGenCXX/float16-declarations.cpp
+++ test/CodeGenCXX/float16-declarations.cpp
@@ -103,7 +103,7 @@
 
   C1 c1(f1l);
 // CHECK-DAG:  [[F1L:%[a-z0-9]+]] = load half, half* %{{.*}}, align 2
-// CHECK-DAG:  call void @_ZN2C1C2EDF16_(%class.C1* %{{.*}}, half %{{.*}})
+// CHECK-DAG:  call void @_ZN2C1C1EDF16_(%class.C1* %{{.*}}, half %{{.*}})
 
   S1<_Float16> s1 = { 132.f16 };
 // CHECK-DAG: @_ZZ4mainE2s1 = private unnamed_addr constant %struct.S1 { half 0xH5820 }, align 2
Index: test/CodeGenCXX/ctor-dtor-alias.cpp
===
--- test/CodeGenCXX/ctor-dtor-alias.cpp
+++ test/CodeGenCXX/ctor-dtor-alias.cpp
@@ -77,11 +77,12 @@
   // CHECK1: call i32 @__cxa_atexit{{.*}}_ZN5test41AD2Ev
   // CHECK1: define linkonce_odr void @_ZN5test41AD2Ev({{.*}} comdat align
 
-  // test that we don't do this optimization at -O0 so that the debugger can
-  // see both destructors.
+  // test that we don't do this optimization at -O0 and call the complete
+  // destructor for B instead. This enables the debugger to see both
+  // destructors.
+  // NOOPT: @_ZN5test41BD1Ev = linkonce_odr alias void {{.*}} @_ZN5test41BD2Ev
   // NOOPT: define internal void @__cxx_global_var_init.2()
-  // NOOPT: call i32 @__cxa_atexit{{.*}}@_ZN5test41BD2Ev
-  // NOOPT: define linkonce_odr void @_ZN5test41BD2Ev({{.*}} comdat align
+  // NOOPT: call i32 @__cxa_atexit{{.*}}@_ZN5test41BD1Ev
   struct A {
 virtual ~A() {}
   };
Index: lib/CodeGen/ItaniumCXXABI.cpp
===
--- lib/CodeGen/ItaniumCXXABI.cpp
+++ lib/CodeGen/ItaniumCXXABI.cpp
@@ -3625,12 +3625,16 @@
   }
   llvm::GlobalValue::LinkageTypes Linkage = CGM.getFunctionLinkage(AliasDecl);
 
-  if (

[PATCH] D46685: [CodeGen] Disable structor optimizations at -O0

[Pavel Labath via Phabricator via cfe-commits]

labath added a comment.

Thank you for the quick review.


Repository:
  rC Clang

https://reviews.llvm.org/D46685



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

[PATCH] D46685: [CodeGen] Disable structor optimizations at -O0

[Pavel Labath via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC332228: [CodeGen] Disable aggressive structor optimizations 
at -O0 (authored by labath, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D46685?vs=146309&id=146578#toc

Repository:
  rC Clang

https://reviews.llvm.org/D46685

Files:
  lib/CodeGen/ItaniumCXXABI.cpp
  test/CodeGenCXX/ctor-dtor-alias.cpp
  test/CodeGenCXX/float16-declarations.cpp


Index: test/CodeGenCXX/float16-declarations.cpp
===
--- test/CodeGenCXX/float16-declarations.cpp
+++ test/CodeGenCXX/float16-declarations.cpp
@@ -103,7 +103,7 @@
 
   C1 c1(f1l);
 // CHECK-DAG:  [[F1L:%[a-z0-9]+]] = load half, half* %{{.*}}, align 2
-// CHECK-DAG:  call void @_ZN2C1C2EDF16_(%class.C1* %{{.*}}, half %{{.*}})
+// CHECK-DAG:  call void @_ZN2C1C1EDF16_(%class.C1* %{{.*}}, half %{{.*}})
 
   S1<_Float16> s1 = { 132.f16 };
 // CHECK-DAG: @_ZZ4mainE2s1 = private unnamed_addr constant %struct.S1 { half 
0xH5820 }, align 2
Index: test/CodeGenCXX/ctor-dtor-alias.cpp
===
--- test/CodeGenCXX/ctor-dtor-alias.cpp
+++ test/CodeGenCXX/ctor-dtor-alias.cpp
@@ -77,11 +77,12 @@
   // CHECK1: call i32 @__cxa_atexit{{.*}}_ZN5test41AD2Ev
   // CHECK1: define linkonce_odr void @_ZN5test41AD2Ev({{.*}} comdat align
 
-  // test that we don't do this optimization at -O0 so that the debugger can
-  // see both destructors.
+  // test that we don't do this optimization at -O0 and call the complete
+  // destructor for B instead. This enables the debugger to see both
+  // destructors.
+  // NOOPT: @_ZN5test41BD1Ev = linkonce_odr alias void {{.*}} @_ZN5test41BD2Ev
   // NOOPT: define internal void @__cxx_global_var_init.2()
-  // NOOPT: call i32 @__cxa_atexit{{.*}}@_ZN5test41BD2Ev
-  // NOOPT: define linkonce_odr void @_ZN5test41BD2Ev({{.*}} comdat align
+  // NOOPT: call i32 @__cxa_atexit{{.*}}@_ZN5test41BD1Ev
   struct A {
 virtual ~A() {}
   };
Index: lib/CodeGen/ItaniumCXXABI.cpp
===
--- lib/CodeGen/ItaniumCXXABI.cpp
+++ lib/CodeGen/ItaniumCXXABI.cpp
@@ -3630,12 +3630,16 @@
   }
   llvm::GlobalValue::LinkageTypes Linkage = CGM.getFunctionLinkage(AliasDecl);
 
-  if (llvm::GlobalValue::isDiscardableIfUnused(Linkage))
-return StructorCodegen::RAUW;
-
-  // FIXME: Should we allow available_externally aliases?
-  if (!llvm::GlobalAlias::isValidLinkage(Linkage))
-return StructorCodegen::RAUW;
+  // Only use RAUW in optimized code, as it makes the complete structor symbol
+  // disappear completely, which degrades debugging experience.
+  if (CGM.getCodeGenOpts().OptimizationLevel > 0) {
+if (llvm::GlobalValue::isDiscardableIfUnused(Linkage))
+  return StructorCodegen::RAUW;
+
+// FIXME: Should we allow available_externally aliases?
+if (!llvm::GlobalAlias::isValidLinkage(Linkage))
+  return StructorCodegen::RAUW;
+  }
 
   if (llvm::GlobalValue::isWeakForLinker(Linkage)) {
 // Only ELF and wasm support COMDATs with arbitrary names (C5/D5).


Index: test/CodeGenCXX/float16-declarations.cpp
===
--- test/CodeGenCXX/float16-declarations.cpp
+++ test/CodeGenCXX/float16-declarations.cpp
@@ -103,7 +103,7 @@
 
   C1 c1(f1l);
 // CHECK-DAG:  [[F1L:%[a-z0-9]+]] = load half, half* %{{.*}}, align 2
-// CHECK-DAG:  call void @_ZN2C1C2EDF16_(%class.C1* %{{.*}}, half %{{.*}})
+// CHECK-DAG:  call void @_ZN2C1C1EDF16_(%class.C1* %{{.*}}, half %{{.*}})
 
   S1<_Float16> s1 = { 132.f16 };
 // CHECK-DAG: @_ZZ4mainE2s1 = private unnamed_addr constant %struct.S1 { half 0xH5820 }, align 2
Index: test/CodeGenCXX/ctor-dtor-alias.cpp
===
--- test/CodeGenCXX/ctor-dtor-alias.cpp
+++ test/CodeGenCXX/ctor-dtor-alias.cpp
@@ -77,11 +77,12 @@
   // CHECK1: call i32 @__cxa_atexit{{.*}}_ZN5test41AD2Ev
   // CHECK1: define linkonce_odr void @_ZN5test41AD2Ev({{.*}} comdat align
 
-  // test that we don't do this optimization at -O0 so that the debugger can
-  // see both destructors.
+  // test that we don't do this optimization at -O0 and call the complete
+  // destructor for B instead. This enables the debugger to see both
+  // destructors.
+  // NOOPT: @_ZN5test41BD1Ev = linkonce_odr alias void {{.*}} @_ZN5test41BD2Ev
   // NOOPT: define internal void @__cxx_global_var_init.2()
-  // NOOPT: call i32 @__cxa_atexit{{.*}}@_ZN5test41BD2Ev
-  // NOOPT: define linkonce_odr void @_ZN5test41BD2Ev({{.*}} comdat align
+  // NOOPT: call i32 @__cxa_atexit{{.*}}@_ZN5test41BD1Ev
   struct A {
 virtual ~A() {}
   };
Index: lib/CodeGen/ItaniumCXXABI.cpp
===
--- lib/CodeGen/ItaniumCXXABI.cpp
+++ lib/CodeGen/ItaniumCXXABI.cpp
@@ -3630,12 +3630,16 @@
   }
   llvm::GlobalValue::LinkageTypes Linkage = 

[PATCH] D46685: [CodeGen] Disable structor optimizations at -O0

[Pavel Labath via Phabricator via cfe-commits]

labath added a comment.

I've reverted the patch as it has caused miscompiles on non-trivial inputs. 
I'll update the patch when I gain a better understanding of what is going on.


Repository:
  rC Clang

https://reviews.llvm.org/D46685



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

[PATCH] D46685: [CodeGen] Disable structor optimizations at -O0

[Pavel Labath via Phabricator via cfe-commits]

labath updated this revision to Diff 147315.
labath added a comment.

Updating with a new version of the patch. The problem with the previous version
was that it caused us to use C5/https://reviews.llvm.org/D5 comdats very 
aggressively (at -O0), and this
is not always correct. This uses a more nuanced approach:

- for local symbols we can use aliases because we don't have to worry about 
comdats or other compilation units.
- for linkonce symbols, we must emit each structor separately. Theoretically it 
would be possible to emit a C5 comdat, but then we would need to make sure it 
contains both (in case of destructors, all three) symbols. As linkonce symbols 
are emitted lazily, I did not find an easy way to ensure that this is always 
the case.
- for available_externally, I also emit the structor, as the code seemed to be 
worried about emitting an alias in this case.
- other linkage types are not affected by the optimization level. They either 
get put into a comdat (weak) or get aliased (external).

I also add more thorough tests for this behavior.

Overall I am not sure if this extra complexity is worth the few extra aliases we
can emit this way at -O0. Let me know what you think.


Repository:
  rC Clang

https://reviews.llvm.org/D46685

Files:
  lib/CodeGen/ItaniumCXXABI.cpp
  test/CodeGenCXX/ctor-dtor-alias.cpp
  test/CodeGenCXX/float16-declarations.cpp

Index: test/CodeGenCXX/float16-declarations.cpp
===
--- test/CodeGenCXX/float16-declarations.cpp
+++ test/CodeGenCXX/float16-declarations.cpp
@@ -103,7 +103,7 @@
 
   C1 c1(f1l);
 // CHECK-DAG:  [[F1L:%[a-z0-9]+]] = load half, half* %{{.*}}, align 2
-// CHECK-DAG:  call void @_ZN2C1C2EDF16_(%class.C1* %{{.*}}, half %{{.*}})
+// CHECK-DAG:  call void @_ZN2C1C1EDF16_(%class.C1* %{{.*}}, half %{{.*}})
 
   S1<_Float16> s1 = { 132.f16 };
 // CHECK-DAG: @_ZZ4mainE2s1 = private unnamed_addr constant %struct.S1 { half 0xH5820 }, align 2
Index: test/CodeGenCXX/ctor-dtor-alias.cpp
===
--- test/CodeGenCXX/ctor-dtor-alias.cpp
+++ test/CodeGenCXX/ctor-dtor-alias.cpp
@@ -1,5 +1,7 @@
-// RUN: %clang_cc1 %s -triple i686-linux -emit-llvm -o - -mconstructor-aliases | FileCheck --check-prefix=NOOPT %s
-
+// RUN: %clang_cc1 %s -triple i686-linux -emit-llvm -o - -mconstructor-aliases > %t
+// RUN: FileCheck --check-prefix=NOOPT1 --input-file=%t %s
+// RUN: FileCheck --check-prefix=NOOPT2 --input-file=%t %s
+// RUN: FileCheck --check-prefix=NOOPT3 --input-file=%t %s
 // RUN: %clang_cc1 %s -triple i686-linux -emit-llvm -o - -mconstructor-aliases -O1 -disable-llvm-passes > %t
 // RUN: FileCheck --check-prefix=CHECK1 --input-file=%t %s
 // RUN: FileCheck --check-prefix=CHECK2 --input-file=%t %s
@@ -21,6 +23,13 @@
 // CHECK1: define weak_odr void @_ZN5test16foobarIvED0Ev({{.*}} comdat($_ZN5test16foobarIvED5Ev)
 // CHECK1-NOT: comdat
 
+// This should happen regardless of the opt level.
+// NOOPT1: @_ZN5test16foobarIvEC1Ev = weak_odr alias void {{.*}} @_ZN5test16foobarIvEC2Ev
+// NOOPT1: @_ZN5test16foobarIvED1Ev = weak_odr alias void (%"struct.test1::foobar"*), void (%"struct.test1::foobar"*)* @_ZN5test16foobarIvED2Ev
+// NOOPT1: define weak_odr void @_ZN5test16foobarIvEC2Ev({{.*}} comdat($_ZN5test16foobarIvEC5Ev)
+// NOOPT1: define weak_odr void @_ZN5test16foobarIvED2Ev({{.*}} comdat($_ZN5test16foobarIvED5Ev)
+// NOOPT1: define weak_odr void @_ZN5test16foobarIvED0Ev({{.*}} comdat($_ZN5test16foobarIvED5Ev)
+
 // COFF doesn't support comdats with arbitrary names (C5/D5).
 // COFF: define weak_odr {{.*}} void @_ZN5test16foobarIvEC2Ev({{.*}} comdat align
 // COFF: define weak_odr {{.*}} void @_ZN5test16foobarIvEC1Ev({{.*}} comdat align
@@ -37,12 +46,17 @@
 }
 
 namespace test2 {
-// test that when the destrucor is linkonce_odr we just replace every use of
+// test that when the destructor is linkonce_odr we just replace every use of
 // C1 with C2.
 
 // CHECK1: define internal void @__cxx_global_var_init()
 // CHECK1: call void @_ZN5test26foobarIvEC2Ev
 // CHECK1: define linkonce_odr void @_ZN5test26foobarIvEC2Ev({{.*}} comdat align
+
+// At -O0, we should still emit the complete constructor.
+// NOOPT1: define internal void @__cxx_global_var_init()
+// NOOPT1: call void @_ZN5test26foobarIvEC1Ev
+// NOOPT1: define linkonce_odr void @_ZN5test26foobarIvEC1Ev({{.*}} comdat align
 void g();
 template  struct foobar {
   foobar() { g(); }
@@ -57,6 +71,11 @@
 // CHECK1: define internal void @__cxx_global_var_init.1()
 // CHECK1: call i32 @__cxa_atexit{{.*}}_ZN5test312_GLOBAL__N_11AD2Ev
 // CHECK1: define internal void @_ZN5test312_GLOBAL__N_11AD2Ev(
+
+// We can use an alias for internal symbols at -O0.
+// NOOPT2: _ZN5test312_GLOBAL__N_11BD1Ev = internal alias void {{.*}} @_ZN5test312_GLOBAL__N_11BD2Ev
+// NOOPT2: define internal void @__cxx_global_var_init.1()
+// NOOPT2: call i32 @__cxa_atexit{{.*}}_ZN5test312_GLOBAL__N_11BD1Ev
 namespace {
 struct A {
 

[PATCH] D46685: [CodeGen] Disable structor optimizations at -O0

[Pavel Labath via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rC332839: [CodeGen] Disable aggressive structor optimizations 
at -O0, take 2 (authored by labath, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D46685?vs=147315&id=147765#toc

Repository:
  rC Clang

https://reviews.llvm.org/D46685

Files:
  lib/CodeGen/ItaniumCXXABI.cpp
  test/CodeGenCXX/ctor-dtor-alias.cpp
  test/CodeGenCXX/float16-declarations.cpp

Index: test/CodeGenCXX/ctor-dtor-alias.cpp
===
--- test/CodeGenCXX/ctor-dtor-alias.cpp
+++ test/CodeGenCXX/ctor-dtor-alias.cpp
@@ -1,5 +1,7 @@
-// RUN: %clang_cc1 %s -triple i686-linux -emit-llvm -o - -mconstructor-aliases | FileCheck --check-prefix=NOOPT %s
-
+// RUN: %clang_cc1 %s -triple i686-linux -emit-llvm -o - -mconstructor-aliases > %t
+// RUN: FileCheck --check-prefix=NOOPT1 --input-file=%t %s
+// RUN: FileCheck --check-prefix=NOOPT2 --input-file=%t %s
+// RUN: FileCheck --check-prefix=NOOPT3 --input-file=%t %s
 // RUN: %clang_cc1 %s -triple i686-linux -emit-llvm -o - -mconstructor-aliases -O1 -disable-llvm-passes > %t
 // RUN: FileCheck --check-prefix=CHECK1 --input-file=%t %s
 // RUN: FileCheck --check-prefix=CHECK2 --input-file=%t %s
@@ -21,6 +23,13 @@
 // CHECK1: define weak_odr void @_ZN5test16foobarIvED0Ev({{.*}} comdat($_ZN5test16foobarIvED5Ev)
 // CHECK1-NOT: comdat
 
+// This should happen regardless of the opt level.
+// NOOPT1: @_ZN5test16foobarIvEC1Ev = weak_odr alias void {{.*}} @_ZN5test16foobarIvEC2Ev
+// NOOPT1: @_ZN5test16foobarIvED1Ev = weak_odr alias void (%"struct.test1::foobar"*), void (%"struct.test1::foobar"*)* @_ZN5test16foobarIvED2Ev
+// NOOPT1: define weak_odr void @_ZN5test16foobarIvEC2Ev({{.*}} comdat($_ZN5test16foobarIvEC5Ev)
+// NOOPT1: define weak_odr void @_ZN5test16foobarIvED2Ev({{.*}} comdat($_ZN5test16foobarIvED5Ev)
+// NOOPT1: define weak_odr void @_ZN5test16foobarIvED0Ev({{.*}} comdat($_ZN5test16foobarIvED5Ev)
+
 // COFF doesn't support comdats with arbitrary names (C5/D5).
 // COFF: define weak_odr {{.*}} void @_ZN5test16foobarIvEC2Ev({{.*}} comdat align
 // COFF: define weak_odr {{.*}} void @_ZN5test16foobarIvEC1Ev({{.*}} comdat align
@@ -37,12 +46,17 @@
 }
 
 namespace test2 {
-// test that when the destrucor is linkonce_odr we just replace every use of
+// test that when the destructor is linkonce_odr we just replace every use of
 // C1 with C2.
 
 // CHECK1: define internal void @__cxx_global_var_init()
 // CHECK1: call void @_ZN5test26foobarIvEC2Ev
 // CHECK1: define linkonce_odr void @_ZN5test26foobarIvEC2Ev({{.*}} comdat align
+
+// At -O0, we should still emit the complete constructor.
+// NOOPT1: define internal void @__cxx_global_var_init()
+// NOOPT1: call void @_ZN5test26foobarIvEC1Ev
+// NOOPT1: define linkonce_odr void @_ZN5test26foobarIvEC1Ev({{.*}} comdat align
 void g();
 template  struct foobar {
   foobar() { g(); }
@@ -57,6 +71,11 @@
 // CHECK1: define internal void @__cxx_global_var_init.1()
 // CHECK1: call i32 @__cxa_atexit{{.*}}_ZN5test312_GLOBAL__N_11AD2Ev
 // CHECK1: define internal void @_ZN5test312_GLOBAL__N_11AD2Ev(
+
+// We can use an alias for internal symbols at -O0.
+// NOOPT2: _ZN5test312_GLOBAL__N_11BD1Ev = internal alias void {{.*}} @_ZN5test312_GLOBAL__N_11BD2Ev
+// NOOPT2: define internal void @__cxx_global_var_init.1()
+// NOOPT2: call i32 @__cxa_atexit{{.*}}_ZN5test312_GLOBAL__N_11BD1Ev
 namespace {
 struct A {
   ~A() {}
@@ -77,11 +96,12 @@
   // CHECK1: call i32 @__cxa_atexit{{.*}}_ZN5test41AD2Ev
   // CHECK1: define linkonce_odr void @_ZN5test41AD2Ev({{.*}} comdat align
 
-  // test that we don't do this optimization at -O0 so that the debugger can
-  // see both destructors.
-  // NOOPT: define internal void @__cxx_global_var_init.2()
-  // NOOPT: call i32 @__cxa_atexit{{.*}}@_ZN5test41BD2Ev
-  // NOOPT: define linkonce_odr void @_ZN5test41BD2Ev({{.*}} comdat align
+  // Test that we don't do this optimization at -O0 and call the complete
+  // destructor for B instead. This enables the debugger to see both
+  // destructors.
+  // NOOPT2: define internal void @__cxx_global_var_init.2()
+  // NOOPT2: call i32 @__cxa_atexit{{.*}}@_ZN5test41BD1Ev
+  // NOOPT2: define linkonce_odr void @_ZN5test41BD1Ev({{.*}} comdat align
   struct A {
 virtual ~A() {}
   };
@@ -129,6 +149,11 @@
   // out if we should).
   // pr17875.
   // CHECK3: define void @_ZN5test71BD2Ev
+
+  // At -O0, we should emit both destructors, the complete can be an alias to
+  // the base one.
+  // NOOPT3: @_ZN5test71BD1Ev = alias void {{.*}} @_ZN5test71BD2Ev
+  // NOOPT3: define void @_ZN5test71BD2Ev
   template  struct A {
 ~A() {}
   };
Index: test/CodeGenCXX/float16-declarations.cpp
===
--- test/CodeGenCXX/float16-declarations.cpp
+++ test/CodeGenCXX/float16-declarations.cpp

[PATCH] D51576: Enable DWARF accelerator tables by default when tuning for lldb (-glldb => -gpubnames)

[Pavel Labath via Phabricator via cfe-commits]

labath created this revision.
labath added a reviewer: dblaikie.

DWARF v5 accelerator tables provide a considerable performance
improvement for lldb and will make the default -glldb behavior same on
all targets (right now we emit apple tables on apple targets, but these
are not controlled by -gpubnames, only by -glldb).


Repository:
  rC Clang

https://reviews.llvm.org/D51576

Files:
  lib/Driver/ToolChains/Clang.cpp
  test/Driver/debug-options.c


Index: test/Driver/debug-options.c
===
--- test/Driver/debug-options.c
+++ test/Driver/debug-options.c
@@ -165,6 +165,9 @@
 // RUN: %clang -### -c -gsplit-dwarf %s 2>&1 | FileCheck -check-prefix=GPUB %s
 // RUN: %clang -### -c -gsplit-dwarf -gno-pubnames %s 2>&1 | FileCheck 
-check-prefix=NOPUB %s
 //
+// RUN: %clang -### -c -glldb %s 2>&1 | FileCheck -check-prefix=GPUB %s
+// RUN: %clang -### -c -glldb -gno-pubnames %s 2>&1 | FileCheck 
-check-prefix=NOPUB %s
+//
 // RUN: %clang -### -c -gdwarf-aranges %s 2>&1 | FileCheck 
-check-prefix=GARANGE %s
 //
 // RUN: %clang -### -fdebug-types-section -target x86_64-unknown-linux %s 2>&1 
\
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -3072,7 +3072,7 @@
   const auto *PubnamesArg =
   Args.getLastArg(options::OPT_ggnu_pubnames, 
options::OPT_gno_gnu_pubnames,
   options::OPT_gpubnames, options::OPT_gno_pubnames);
-  if (SplitDWARFArg ||
+  if (SplitDWARFArg || DebuggerTuning == llvm::DebuggerKind::LLDB ||
   (PubnamesArg && checkDebugInfoOption(PubnamesArg, Args, D, TC)))
 if (!PubnamesArg ||
 (!PubnamesArg->getOption().matches(options::OPT_gno_gnu_pubnames) &&


Index: test/Driver/debug-options.c
===
--- test/Driver/debug-options.c
+++ test/Driver/debug-options.c
@@ -165,6 +165,9 @@
 // RUN: %clang -### -c -gsplit-dwarf %s 2>&1 | FileCheck -check-prefix=GPUB %s
 // RUN: %clang -### -c -gsplit-dwarf -gno-pubnames %s 2>&1 | FileCheck -check-prefix=NOPUB %s
 //
+// RUN: %clang -### -c -glldb %s 2>&1 | FileCheck -check-prefix=GPUB %s
+// RUN: %clang -### -c -glldb -gno-pubnames %s 2>&1 | FileCheck -check-prefix=NOPUB %s
+//
 // RUN: %clang -### -c -gdwarf-aranges %s 2>&1 | FileCheck -check-prefix=GARANGE %s
 //
 // RUN: %clang -### -fdebug-types-section -target x86_64-unknown-linux %s 2>&1 \
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -3072,7 +3072,7 @@
   const auto *PubnamesArg =
   Args.getLastArg(options::OPT_ggnu_pubnames, options::OPT_gno_gnu_pubnames,
   options::OPT_gpubnames, options::OPT_gno_pubnames);
-  if (SplitDWARFArg ||
+  if (SplitDWARFArg || DebuggerTuning == llvm::DebuggerKind::LLDB ||
   (PubnamesArg && checkDebugInfoOption(PubnamesArg, Args, D, TC)))
 if (!PubnamesArg ||
 (!PubnamesArg->getOption().matches(options::OPT_gno_gnu_pubnames) &&
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51576: Enable DWARF accelerator tables by default when tuning for lldb (-glldb => -gpubnames)

[Pavel Labath via Phabricator via cfe-commits]

labath added a comment.

In https://reviews.llvm.org/D51576#1223234, @aprantl wrote:

> This is DWARF5+ only, right? (We shouldn't change the preference of Apple 
> accelerator tables for DWARF 4 and earlier).


The interactions here are a bit weird, but the short answer is no, this will 
not affect apple tables in any way.

The long answer is: The type of accelerator tables that get generated gets 
chosen in the back end, based on target triple and dwarf version. If apple 
tables get chosen, then this patch makes no difference because apple tables 
ignore the -gpubnames argument. If dwarf tables get chosen, then they will only 
index the CUs which had the "pubnames" flag set to on (!None). This patch makes 
it such that we automatically set that flag when tuning for lldb, thereby 
obtaining the same default behavior as apple tables (indexing all CUs when 
tuning for lldb).


Repository:
  rC Clang

https://reviews.llvm.org/D51576



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

[PATCH] D51576: Enable DWARF accelerator tables by default when tuning for lldb (-glldb => -gpubnames)

[Pavel Labath via Phabricator via cfe-commits]

labath added a comment.

In https://reviews.llvm.org/D51576#1223596, @clayborg wrote:

> We want to ensure that both Apple and DWARF5 tables never get generated 
> though. That would waste a lot of space. I would say if DWARF5 tables are 
> enabled, then we need ensure we disable Apple tables.


That's already taken care of. The back end will always generate at most one 
kind of accel tables.


Repository:
  rC Clang

https://reviews.llvm.org/D51576



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

[PATCH] D51576: Enable DWARF accelerator tables by default when tuning for lldb (-glldb => -gpubnames)

[Pavel Labath via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL341472: Enable DWARF accelerator tables by default when 
tuning for lldb (-glldb =>… (authored by labath, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D51576

Files:
  cfe/trunk/lib/Driver/ToolChains/Clang.cpp
  cfe/trunk/test/Driver/debug-options.c


Index: cfe/trunk/test/Driver/debug-options.c
===
--- cfe/trunk/test/Driver/debug-options.c
+++ cfe/trunk/test/Driver/debug-options.c
@@ -165,6 +165,9 @@
 // RUN: %clang -### -c -gsplit-dwarf %s 2>&1 | FileCheck -check-prefix=GPUB %s
 // RUN: %clang -### -c -gsplit-dwarf -gno-pubnames %s 2>&1 | FileCheck 
-check-prefix=NOPUB %s
 //
+// RUN: %clang -### -c -glldb %s 2>&1 | FileCheck -check-prefix=GPUB %s
+// RUN: %clang -### -c -glldb -gno-pubnames %s 2>&1 | FileCheck 
-check-prefix=NOPUB %s
+//
 // RUN: %clang -### -c -gdwarf-aranges %s 2>&1 | FileCheck 
-check-prefix=GARANGE %s
 //
 // RUN: %clang -### -fdebug-types-section -target x86_64-unknown-linux %s 2>&1 
\
Index: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/Clang.cpp
+++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp
@@ -3072,7 +3072,7 @@
   const auto *PubnamesArg =
   Args.getLastArg(options::OPT_ggnu_pubnames, 
options::OPT_gno_gnu_pubnames,
   options::OPT_gpubnames, options::OPT_gno_pubnames);
-  if (SplitDWARFArg ||
+  if (SplitDWARFArg || DebuggerTuning == llvm::DebuggerKind::LLDB ||
   (PubnamesArg && checkDebugInfoOption(PubnamesArg, Args, D, TC)))
 if (!PubnamesArg ||
 (!PubnamesArg->getOption().matches(options::OPT_gno_gnu_pubnames) &&


Index: cfe/trunk/test/Driver/debug-options.c
===
--- cfe/trunk/test/Driver/debug-options.c
+++ cfe/trunk/test/Driver/debug-options.c
@@ -165,6 +165,9 @@
 // RUN: %clang -### -c -gsplit-dwarf %s 2>&1 | FileCheck -check-prefix=GPUB %s
 // RUN: %clang -### -c -gsplit-dwarf -gno-pubnames %s 2>&1 | FileCheck -check-prefix=NOPUB %s
 //
+// RUN: %clang -### -c -glldb %s 2>&1 | FileCheck -check-prefix=GPUB %s
+// RUN: %clang -### -c -glldb -gno-pubnames %s 2>&1 | FileCheck -check-prefix=NOPUB %s
+//
 // RUN: %clang -### -c -gdwarf-aranges %s 2>&1 | FileCheck -check-prefix=GARANGE %s
 //
 // RUN: %clang -### -fdebug-types-section -target x86_64-unknown-linux %s 2>&1 \
Index: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/Clang.cpp
+++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp
@@ -3072,7 +3072,7 @@
   const auto *PubnamesArg =
   Args.getLastArg(options::OPT_ggnu_pubnames, options::OPT_gno_gnu_pubnames,
   options::OPT_gpubnames, options::OPT_gno_pubnames);
-  if (SplitDWARFArg ||
+  if (SplitDWARFArg || DebuggerTuning == llvm::DebuggerKind::LLDB ||
   (PubnamesArg && checkDebugInfoOption(PubnamesArg, Args, D, TC)))
 if (!PubnamesArg ||
 (!PubnamesArg->getOption().matches(options::OPT_gno_gnu_pubnames) &&
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49594: [DebugInfo] Emit diagnostics when enabling -fdebug-types-section on non-linux target.

[Pavel Labath via Phabricator via cfe-commits]

labath added a comment.

I believe debug_types is used on non-linux targets as well. Judging by the 
other review all ELF targets should at least have a chance of working, so maybe 
key the error off of that?


Repository:
  rC Clang

https://reviews.llvm.org/D49594



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

[PATCH] D49989: WIP: [CodeGen] Disable aggressive structor optimizations at -O0, take 4

[Pavel Labath via Phabricator via cfe-commits]

labath created this revision.
Herald added subscribers: cfe-commits, aheejin.

This version of the patch attempts to mitigate the code size regressions
caused by the previous versions of this patch 
(https://reviews.llvm.org/D46685). It does this by
using aliases and C5/https://reviews.llvm.org/D5 comdats more aggressively.

This is not completely trivial, as linkonce_odr structors (these are
othe ones that were previously causing the blowup) are emitte lazily,
only when they are referenced, but we are only able to use the comdat if
all of the elements that should go into it are emitted. This is
particularly tricky for virtual desctructors as here the comdat must
include all three versions (complete, base, deleting).

As we do not (?) want this to impact the decisions on whether to emit
certain variants or not (and the full set of emitted versions is not
easily accessible from the place where we make the comdat decision), we
have to opportunistically wait for all required versions to be emitted.
If they are, we coalesce them into a comdat.


Repository:
  rC Clang

https://reviews.llvm.org/D49989

Files:
  lib/CodeGen/ItaniumCXXABI.cpp
  test/CodeGenCXX/ctor-dtor-alias.cpp
  test/CodeGenCXX/float16-declarations.cpp

Index: test/CodeGenCXX/float16-declarations.cpp
===
--- test/CodeGenCXX/float16-declarations.cpp
+++ test/CodeGenCXX/float16-declarations.cpp
@@ -103,7 +103,7 @@
 
   C1 c1(f1l);
 // CHECK-DAG:  [[F1L:%[a-z0-9]+]] = load half, half* %{{.*}}, align 2
-// CHECK-DAG:  call void @_ZN2C1C2EDF16_(%class.C1* %{{.*}}, half %{{.*}})
+// CHECK-DAG:  call void @_ZN2C1C1EDF16_(%class.C1* %{{.*}}, half %{{.*}})
 
   S1<_Float16> s1 = { 132.f16 };
 // CHECK-DAG: @_ZZ4mainE2s1 = private unnamed_addr constant %struct.S1 { half 0xH5820 }, align 2
Index: test/CodeGenCXX/ctor-dtor-alias.cpp
===
--- test/CodeGenCXX/ctor-dtor-alias.cpp
+++ test/CodeGenCXX/ctor-dtor-alias.cpp
@@ -1,5 +1,11 @@
-// RUN: %clang_cc1 %s -triple i686-linux -emit-llvm -o - -mconstructor-aliases | FileCheck --check-prefix=NOOPT %s
-
+// RUN: %clang_cc1 %s -triple i686-linux -emit-llvm -o - -mconstructor-aliases > %t
+// RUN: FileCheck --check-prefix=NOOPT1 --input-file=%t %s
+// RUN: FileCheck --check-prefix=NOOPT2 --input-file=%t %s
+// RUN: FileCheck --check-prefix=NOOPT3 --implicit-check-not=_ZN6test2a6foobarIvED0Ev --input-file=%t %s
+// RUN: FileCheck --check-prefix=NOOPT4 --input-file=%t %s
+// RUN: FileCheck --check-prefix=NOOPT5 --input-file=%t %s
+// RUN: FileCheck --check-prefix=NOOPT6 --implicit-check-not=_ZN6test4a1BD0Ev --input-file=%t %s
+// RUN: FileCheck --check-prefix=NOOPT7 --input-file=%t %s
 // RUN: %clang_cc1 %s -triple i686-linux -emit-llvm -o - -mconstructor-aliases -O1 -disable-llvm-passes > %t
 // RUN: FileCheck --check-prefix=CHECK1 --input-file=%t %s
 // RUN: FileCheck --check-prefix=CHECK2 --input-file=%t %s
@@ -21,6 +27,13 @@
 // CHECK1: define weak_odr void @_ZN5test16foobarIvED0Ev({{.*}} comdat($_ZN5test16foobarIvED5Ev)
 // CHECK1-NOT: comdat
 
+// This should happen regardless of the opt level.
+// NOOPT1: @_ZN5test16foobarIvEC1Ev = weak_odr unnamed_addr alias void {{.*}} @_ZN5test16foobarIvEC2Ev
+// NOOPT1: @_ZN5test16foobarIvED1Ev = weak_odr unnamed_addr alias void (%"struct.test1::foobar"*), void (%"struct.test1::foobar"*)* @_ZN5test16foobarIvED2Ev
+// NOOPT1: define weak_odr void @_ZN5test16foobarIvEC2Ev({{.*}} comdat($_ZN5test16foobarIvEC5Ev)
+// NOOPT1: define weak_odr void @_ZN5test16foobarIvED2Ev({{.*}} comdat($_ZN5test16foobarIvED5Ev)
+// NOOPT1: define weak_odr void @_ZN5test16foobarIvED0Ev({{.*}} comdat($_ZN5test16foobarIvED5Ev)
+
 // COFF doesn't support comdats with arbitrary names (C5/D5).
 // COFF: define weak_odr {{.*}} void @_ZN5test16foobarIvEC2Ev({{.*}} comdat align
 // COFF: define weak_odr {{.*}} void @_ZN5test16foobarIvEC1Ev({{.*}} comdat align
@@ -37,26 +50,52 @@
 }
 
 namespace test2 {
-// test that when the destrucor is linkonce_odr we just replace every use of
+// test that when the destructor is linkonce_odr we just replace every use of
 // C1 with C2.
 
 // CHECK1: define internal void @__cxx_global_var_init()
 // CHECK1: call void @_ZN5test26foobarIvEC2Ev
 // CHECK1: define linkonce_odr void @_ZN5test26foobarIvEC2Ev({{.*}} comdat align
+
+// At -O0, we still emit both symbols, but C1 is an alias to save space.
+// NOOPT2: @_ZN5test26foobarIvEC1Ev = linkonce_odr unnamed_addr alias void {{.*}} @_ZN5test26foobarIvEC2Ev
+// NOOPT2: define internal void @__cxx_global_var_init()
+// NOOPT2: call void @_ZN5test26foobarIvEC1Ev
+// NOOPT2: define linkonce_odr void @_ZN5test26foobarIvEC2Ev({{.*}} comdat($_ZN5test26foobarIvEC5Ev) align
 void g();
 template  struct foobar {
   foobar() { g(); }
 };
 foobar x;
 }
 
+namespace test2a {
+// Similar to test2, but check destructors. As D0 is not referenced and not
+// virtual, it shouldn't be emitted, nor placed into a co

[PATCH] D41387: Remove llvm::MemoryBuffer const_casts

[Pavel Labath via Phabricator via cfe-commits]

labath created this revision.
labath added reviewers: dblaikie, rsmith.

llvm has grown a WritableMemoryBuffer class, which is convertible
(inherits from) a MemoryBuffer. We can use it to avoid conts_casting the
buffer contents when we want to write to it.


Repository:
  rC Clang

https://reviews.llvm.org/D41387

Files:
  lib/Basic/SourceManager.cpp
  lib/Lex/Preprocessor.cpp


Index: lib/Lex/Preprocessor.cpp
===
--- lib/Lex/Preprocessor.cpp
+++ lib/Lex/Preprocessor.cpp
@@ -420,10 +420,9 @@
   CodeCompletionFile = File;
   CodeCompletionOffset = Position - Buffer->getBufferStart();
 
-  std::unique_ptr NewBuffer =
-  MemoryBuffer::getNewUninitMemBuffer(Buffer->getBufferSize() + 1,
-  Buffer->getBufferIdentifier());
-  char *NewBuf = const_cast(NewBuffer->getBufferStart());
+  auto NewBuffer = llvm::WritableMemoryBuffer::getNewUninitMemBuffer(
+  Buffer->getBufferSize() + 1, Buffer->getBufferIdentifier());
+  char *NewBuf = NewBuffer->getBufferStart();
   char *NewPos = std::copy(Buffer->getBufferStart(), Position, NewBuf);
   *NewPos = '\0';
   std::copy(Position, Buffer->getBufferEnd(), NewPos+1);
Index: lib/Basic/SourceManager.cpp
===
--- lib/Basic/SourceManager.cpp
+++ lib/Basic/SourceManager.cpp
@@ -125,11 +125,12 @@
   // possible.
   if (!BufferOrError) {
 StringRef FillStr("<<>>\n");
-Buffer.setPointer(MemoryBuffer::getNewUninitMemBuffer(
-  ContentsEntry->getSize(), "").release());
-char *Ptr = const_cast(Buffer.getPointer()->getBufferStart());
+auto BackupBuffer = llvm::WritableMemoryBuffer::getNewUninitMemBuffer(
+ContentsEntry->getSize(), "");
+char *Ptr = BackupBuffer->getBufferStart();
 for (unsigned i = 0, e = ContentsEntry->getSize(); i != e; ++i)
   Ptr[i] = FillStr[i % FillStr.size()];
+Buffer.setPointer(BackupBuffer.release());
 
 if (Diag.isDiagnosticInFlight())
   Diag.SetDelayedDiagnostic(diag::err_cannot_open_file,


Index: lib/Lex/Preprocessor.cpp
===
--- lib/Lex/Preprocessor.cpp
+++ lib/Lex/Preprocessor.cpp
@@ -420,10 +420,9 @@
   CodeCompletionFile = File;
   CodeCompletionOffset = Position - Buffer->getBufferStart();
 
-  std::unique_ptr NewBuffer =
-  MemoryBuffer::getNewUninitMemBuffer(Buffer->getBufferSize() + 1,
-  Buffer->getBufferIdentifier());
-  char *NewBuf = const_cast(NewBuffer->getBufferStart());
+  auto NewBuffer = llvm::WritableMemoryBuffer::getNewUninitMemBuffer(
+  Buffer->getBufferSize() + 1, Buffer->getBufferIdentifier());
+  char *NewBuf = NewBuffer->getBufferStart();
   char *NewPos = std::copy(Buffer->getBufferStart(), Position, NewBuf);
   *NewPos = '\0';
   std::copy(Position, Buffer->getBufferEnd(), NewPos+1);
Index: lib/Basic/SourceManager.cpp
===
--- lib/Basic/SourceManager.cpp
+++ lib/Basic/SourceManager.cpp
@@ -125,11 +125,12 @@
   // possible.
   if (!BufferOrError) {
 StringRef FillStr("<<>>\n");
-Buffer.setPointer(MemoryBuffer::getNewUninitMemBuffer(
-  ContentsEntry->getSize(), "").release());
-char *Ptr = const_cast(Buffer.getPointer()->getBufferStart());
+auto BackupBuffer = llvm::WritableMemoryBuffer::getNewUninitMemBuffer(
+ContentsEntry->getSize(), "");
+char *Ptr = BackupBuffer->getBufferStart();
 for (unsigned i = 0, e = ContentsEntry->getSize(); i != e; ++i)
   Ptr[i] = FillStr[i % FillStr.size()];
+Buffer.setPointer(BackupBuffer.release());
 
 if (Diag.isDiagnosticInFlight())
   Diag.SetDelayedDiagnostic(diag::err_cannot_open_file,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41387: Remove llvm::MemoryBuffer const_casts

[Pavel Labath via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL321167: Remove llvm::MemoryBuffer const_casts (authored by 
labath, committed by ).

Repository:
  rL LLVM

https://reviews.llvm.org/D41387

Files:
  cfe/trunk/lib/Basic/SourceManager.cpp
  cfe/trunk/lib/Lex/Preprocessor.cpp


Index: cfe/trunk/lib/Lex/Preprocessor.cpp
===
--- cfe/trunk/lib/Lex/Preprocessor.cpp
+++ cfe/trunk/lib/Lex/Preprocessor.cpp
@@ -420,10 +420,9 @@
   CodeCompletionFile = File;
   CodeCompletionOffset = Position - Buffer->getBufferStart();
 
-  std::unique_ptr NewBuffer =
-  MemoryBuffer::getNewUninitMemBuffer(Buffer->getBufferSize() + 1,
-  Buffer->getBufferIdentifier());
-  char *NewBuf = const_cast(NewBuffer->getBufferStart());
+  auto NewBuffer = llvm::WritableMemoryBuffer::getNewUninitMemBuffer(
+  Buffer->getBufferSize() + 1, Buffer->getBufferIdentifier());
+  char *NewBuf = NewBuffer->getBufferStart();
   char *NewPos = std::copy(Buffer->getBufferStart(), Position, NewBuf);
   *NewPos = '\0';
   std::copy(Position, Buffer->getBufferEnd(), NewPos+1);
Index: cfe/trunk/lib/Basic/SourceManager.cpp
===
--- cfe/trunk/lib/Basic/SourceManager.cpp
+++ cfe/trunk/lib/Basic/SourceManager.cpp
@@ -125,11 +125,12 @@
   // possible.
   if (!BufferOrError) {
 StringRef FillStr("<<>>\n");
-Buffer.setPointer(MemoryBuffer::getNewUninitMemBuffer(
-  ContentsEntry->getSize(), "").release());
-char *Ptr = const_cast(Buffer.getPointer()->getBufferStart());
+auto BackupBuffer = llvm::WritableMemoryBuffer::getNewUninitMemBuffer(
+ContentsEntry->getSize(), "");
+char *Ptr = BackupBuffer->getBufferStart();
 for (unsigned i = 0, e = ContentsEntry->getSize(); i != e; ++i)
   Ptr[i] = FillStr[i % FillStr.size()];
+Buffer.setPointer(BackupBuffer.release());
 
 if (Diag.isDiagnosticInFlight())
   Diag.SetDelayedDiagnostic(diag::err_cannot_open_file,


Index: cfe/trunk/lib/Lex/Preprocessor.cpp
===
--- cfe/trunk/lib/Lex/Preprocessor.cpp
+++ cfe/trunk/lib/Lex/Preprocessor.cpp
@@ -420,10 +420,9 @@
   CodeCompletionFile = File;
   CodeCompletionOffset = Position - Buffer->getBufferStart();
 
-  std::unique_ptr NewBuffer =
-  MemoryBuffer::getNewUninitMemBuffer(Buffer->getBufferSize() + 1,
-  Buffer->getBufferIdentifier());
-  char *NewBuf = const_cast(NewBuffer->getBufferStart());
+  auto NewBuffer = llvm::WritableMemoryBuffer::getNewUninitMemBuffer(
+  Buffer->getBufferSize() + 1, Buffer->getBufferIdentifier());
+  char *NewBuf = NewBuffer->getBufferStart();
   char *NewPos = std::copy(Buffer->getBufferStart(), Position, NewBuf);
   *NewPos = '\0';
   std::copy(Position, Buffer->getBufferEnd(), NewPos+1);
Index: cfe/trunk/lib/Basic/SourceManager.cpp
===
--- cfe/trunk/lib/Basic/SourceManager.cpp
+++ cfe/trunk/lib/Basic/SourceManager.cpp
@@ -125,11 +125,12 @@
   // possible.
   if (!BufferOrError) {
 StringRef FillStr("<<>>\n");
-Buffer.setPointer(MemoryBuffer::getNewUninitMemBuffer(
-  ContentsEntry->getSize(), "").release());
-char *Ptr = const_cast(Buffer.getPointer()->getBufferStart());
+auto BackupBuffer = llvm::WritableMemoryBuffer::getNewUninitMemBuffer(
+ContentsEntry->getSize(), "");
+char *Ptr = BackupBuffer->getBufferStart();
 for (unsigned i = 0, e = ContentsEntry->getSize(); i != e; ++i)
   Ptr[i] = FillStr[i % FillStr.size()];
+Buffer.setPointer(BackupBuffer.release());
 
 if (Diag.isDiagnosticInFlight())
   Diag.SetDelayedDiagnostic(diag::err_cannot_open_file,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47886: Move VersionTuple from clang/Basic to llvm/Support, clang part

[Pavel Labath via Phabricator via cfe-commits]

labath created this revision.
labath added reviewers: erik.pilkington, zturner.
Herald added a subscriber: mgorny.

This kind of functionality is useful to other project apart from clang.
LLDB works with version numbers a lot, but it does not have a convenient
abstraction for this. Moving this class to a lower level library allows
it to be freely used within LLDB.

Since this class is used in a lot of places, and it used to be in the
clang namespace, it seemed appropriate to add it to the list of adopted
classes in LLVM.h to avoid prefixing all uses with "llvm::".


Repository:
  rC Clang

https://reviews.llvm.org/D47886

Files:
  include/clang/AST/Attr.h
  include/clang/AST/Availability.h
  include/clang/AST/DeclBase.h
  include/clang/AST/ExprObjC.h
  include/clang/Basic/AlignedAllocation.h
  include/clang/Basic/LLVM.h
  include/clang/Basic/ObjCRuntime.h
  include/clang/Basic/TargetInfo.h
  include/clang/Basic/VersionTuple.h
  include/clang/Driver/ToolChain.h
  include/clang/Parse/Parser.h
  include/clang/Sema/AttributeList.h
  include/clang/Serialization/ASTReader.h
  include/clang/Serialization/ASTWriter.h
  lib/AST/DeclBase.cpp
  lib/Basic/CMakeLists.txt
  lib/Basic/ObjCRuntime.cpp
  lib/Basic/VersionTuple.cpp
  lib/Driver/ToolChain.cpp
  lib/Driver/ToolChains/Cuda.h
  lib/Frontend/CompilerInvocation.cpp
  lib/Serialization/ASTReader.cpp
  lib/Serialization/ASTWriter.cpp

Index: lib/Serialization/ASTWriter.cpp
===
--- lib/Serialization/ASTWriter.cpp
+++ lib/Serialization/ASTWriter.cpp
@@ -53,7 +53,6 @@
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Basic/TargetOptions.h"
 #include "clang/Basic/Version.h"
-#include "clang/Basic/VersionTuple.h"
 #include "clang/Lex/HeaderSearch.h"
 #include "clang/Lex/HeaderSearchOptions.h"
 #include "clang/Lex/MacroInfo.h"
@@ -97,6 +96,7 @@
 #include "llvm/Support/OnDiskHashTable.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/SHA1.h"
+#include "llvm/Support/VersionTuple.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
 #include 
Index: lib/Serialization/ASTReader.cpp
===
--- lib/Serialization/ASTReader.cpp
+++ lib/Serialization/ASTReader.cpp
@@ -61,7 +61,6 @@
 #include "clang/Basic/TargetOptions.h"
 #include "clang/Basic/TokenKinds.h"
 #include "clang/Basic/Version.h"
-#include "clang/Basic/VersionTuple.h"
 #include "clang/Frontend/PCHContainerOperations.h"
 #include "clang/Lex/HeaderSearch.h"
 #include "clang/Lex/HeaderSearchOptions.h"
@@ -104,8 +103,8 @@
 #include "llvm/ADT/iterator_range.h"
 #include "llvm/Bitcode/BitstreamReader.h"
 #include "llvm/Support/Casting.h"
-#include "llvm/Support/Compression.h"
 #include "llvm/Support/Compiler.h"
+#include "llvm/Support/Compression.h"
 #include "llvm/Support/DJB.h"
 #include "llvm/Support/Endian.h"
 #include "llvm/Support/Error.h"
@@ -115,6 +114,7 @@
 #include "llvm/Support/Path.h"
 #include "llvm/Support/SaveAndRestore.h"
 #include "llvm/Support/Timer.h"
+#include "llvm/Support/VersionTuple.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
 #include 
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -23,7 +23,6 @@
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/TargetOptions.h"
 #include "clang/Basic/Version.h"
-#include "clang/Basic/VersionTuple.h"
 #include "clang/Basic/VirtualFileSystem.h"
 #include "clang/Basic/Visibility.h"
 #include "clang/Basic/XRayInstr.h"
@@ -76,6 +75,7 @@
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/Regex.h"
+#include "llvm/Support/VersionTuple.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Target/TargetOptions.h"
 #include 
Index: lib/Driver/ToolChains/Cuda.h
===
--- lib/Driver/ToolChains/Cuda.h
+++ lib/Driver/ToolChains/Cuda.h
@@ -11,14 +11,14 @@
 #define LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_CUDA_H
 
 #include "clang/Basic/Cuda.h"
-#include "clang/Basic/VersionTuple.h"
 #include "clang/Driver/Action.h"
 #include "clang/Driver/Multilib.h"
-#include "clang/Driver/ToolChain.h"
 #include "clang/Driver/Tool.h"
+#include "clang/Driver/ToolChain.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/Support/Compiler.h"
+#include "llvm/Support/VersionTuple.h"
 #include 
 #include 
 
Index: lib/Driver/ToolChain.cpp
===
--- lib/Driver/ToolChain.cpp
+++ lib/Driver/ToolChain.cpp
@@ -13,7 +13,6 @@
 #include "ToolChains/Clang.h"
 #include "clang/Basic/ObjCRuntime.h"
 #include "clang/Basic/Sanitizers.h"
-#include "clang/Basic/VersionTuple.h"
 #include "clang/Basic/VirtualFileSystem.h"
 #include "clang/Config/config.h"
 #include "clang/Driver/Action.h"
@@ -29,16 +28,17 @@
 #include "llvm/ADT/Trip

[PATCH] D47887: Move VersionTuple from clang/Basic to llvm/Support, llvm part

[Pavel Labath via Phabricator via cfe-commits]

labath created this revision.
labath added reviewers: zturner, erik.pilkington.
Herald added a subscriber: mgorny.

This kind of functionality is useful to other project apart from clang.
LLDB works with version numbers a lot, but it does not have a convenient
abstraction for this. Moving this class to a lower level library allows
it to be freely used within LLDB.

The code is an identical copy of the version from clang (apart from
namespace change and re-clang-formatting).

I didn't find any tests specific for this class, so I wrote a couple of
quick ones for the more interesting bits of functionality.


Repository:
  rL LLVM

https://reviews.llvm.org/D47887

Files:
  include/llvm/Support/VersionTuple.h
  lib/Support/CMakeLists.txt
  lib/Support/VersionTuple.cpp
  unittests/Support/CMakeLists.txt
  unittests/Support/VersionTupleTest.cpp

Index: unittests/Support/VersionTupleTest.cpp
===
--- /dev/null
+++ unittests/Support/VersionTupleTest.cpp
@@ -0,0 +1,50 @@
+//===- VersionTupleTests.cpp - Version Number Handling Tests --===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "llvm/Support/VersionTuple.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+
+TEST(VersionTuple, getAsString) {
+  EXPECT_EQ("0", VersionTuple().getAsString());
+  EXPECT_EQ("1", VersionTuple(1).getAsString());
+  EXPECT_EQ("1.2", VersionTuple(1, 2).getAsString());
+  EXPECT_EQ("1.2.3", VersionTuple(1, 2, 3).getAsString());
+  EXPECT_EQ("1.2.3.4", VersionTuple(1, 2, 3, 4).getAsString());
+}
+
+TEST(VersionTuple, tryParse) {
+  VersionTuple VT;
+
+  EXPECT_FALSE(VT.tryParse("1"));
+  EXPECT_EQ("1", VT.getAsString());
+
+  EXPECT_FALSE(VT.tryParse("1.2"));
+  EXPECT_EQ("1.2", VT.getAsString());
+
+  EXPECT_FALSE(VT.tryParse("1.2.3"));
+  EXPECT_EQ("1.2.3", VT.getAsString());
+
+  EXPECT_FALSE(VT.tryParse("1.2.3.4"));
+  EXPECT_EQ("1.2.3.4", VT.getAsString());
+
+  EXPECT_TRUE(VT.tryParse(""));
+  EXPECT_TRUE(VT.tryParse("1."));
+  EXPECT_TRUE(VT.tryParse("1.2."));
+  EXPECT_TRUE(VT.tryParse("1.2.3."));
+  EXPECT_TRUE(VT.tryParse("1.2.3.4."));
+  EXPECT_TRUE(VT.tryParse("1.2.3.4.5"));
+  EXPECT_TRUE(VT.tryParse("1-2"));
+  EXPECT_TRUE(VT.tryParse("1+2"));
+  EXPECT_TRUE(VT.tryParse(".1"));
+  EXPECT_TRUE(VT.tryParse(" 1"));
+  EXPECT_TRUE(VT.tryParse("1 "));
+  EXPECT_TRUE(VT.tryParse("."));
+}
Index: unittests/Support/CMakeLists.txt
===
--- unittests/Support/CMakeLists.txt
+++ unittests/Support/CMakeLists.txt
@@ -61,6 +61,7 @@
   TrailingObjectsTest.cpp
   TrigramIndexTest.cpp
   UnicodeTest.cpp
+  VersionTupleTest.cpp
   YAMLIOTest.cpp
   YAMLParserTest.cpp
   formatted_raw_ostream_test.cpp
Index: lib/Support/VersionTuple.cpp
===
--- /dev/null
+++ lib/Support/VersionTuple.cpp
@@ -0,0 +1,110 @@
+//===- VersionTuple.cpp - Version Number Handling ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// This file implements the VersionTuple class, which represents a version in
+// the form major[.minor[.subminor]].
+//
+//===--===//
+#include "llvm/Support/VersionTuple.h"
+#include "llvm/Support/raw_ostream.h"
+
+using namespace llvm;
+
+std::string VersionTuple::getAsString() const {
+  std::string Result;
+  {
+llvm::raw_string_ostream Out(Result);
+Out << *this;
+  }
+  return Result;
+}
+
+raw_ostream &llvm::operator<<(raw_ostream &Out, const VersionTuple &V) {
+  Out << V.getMajor();
+  if (Optional Minor = V.getMinor())
+Out << '.' << *Minor;
+  if (Optional Subminor = V.getSubminor())
+Out << '.' << *Subminor;
+  if (Optional Build = V.getBuild())
+Out << '.' << *Build;
+  return Out;
+}
+
+static bool parseInt(StringRef &input, unsigned &value) {
+  assert(value == 0);
+  if (input.empty())
+return true;
+
+  char next = input[0];
+  input = input.substr(1);
+  if (next < '0' || next > '9')
+return true;
+  value = (unsigned)(next - '0');
+
+  while (!input.empty()) {
+next = input[0];
+if (next < '0' || next > '9')
+  return false;
+input = input.substr(1);
+value = value * 10 + (unsigned)(next - '0');
+  }
+
+  return false;
+}
+
+bool VersionTuple::tryParse(StringRef input) {
+  unsigned major = 0, minor = 0, micro = 0, build = 0;
+
+  // Parse the major version, [0-9]+
+  if (parseInt(input, major))
+return true;
+
+  if (input.empty

[PATCH] D47887: Move VersionTuple from clang/Basic to llvm/Support, llvm part

[Pavel Labath via Phabricator via cfe-commits]

labath added a comment.

Yea, one day I'll have to try that out. I'm just putting it off cause it would 
nuke all my branches and build folders :/

FWIW, the llvm part can actually be committed without breaking clang or anyone.


Repository:
  rL LLVM

https://reviews.llvm.org/D47887



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

[PATCH] D47887: Move VersionTuple from clang/Basic to llvm/Support, llvm part

[Pavel Labath via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL334399: Move VersionTuple from clang/Basic to llvm/Support 
(authored by labath, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D47887?vs=150339&id=150701#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D47887

Files:
  cfe/trunk/include/clang/AST/Attr.h
  cfe/trunk/include/clang/AST/Availability.h
  cfe/trunk/include/clang/AST/DeclBase.h
  cfe/trunk/include/clang/AST/ExprObjC.h
  cfe/trunk/include/clang/Basic/AlignedAllocation.h
  cfe/trunk/include/clang/Basic/LLVM.h
  cfe/trunk/include/clang/Basic/ObjCRuntime.h
  cfe/trunk/include/clang/Basic/TargetInfo.h
  cfe/trunk/include/clang/Basic/VersionTuple.h
  cfe/trunk/include/clang/Driver/ToolChain.h
  cfe/trunk/include/clang/Parse/Parser.h
  cfe/trunk/include/clang/Sema/AttributeList.h
  cfe/trunk/include/clang/Serialization/ASTReader.h
  cfe/trunk/include/clang/Serialization/ASTWriter.h
  cfe/trunk/lib/AST/DeclBase.cpp
  cfe/trunk/lib/Basic/CMakeLists.txt
  cfe/trunk/lib/Basic/ObjCRuntime.cpp
  cfe/trunk/lib/Basic/VersionTuple.cpp
  cfe/trunk/lib/Driver/ToolChain.cpp
  cfe/trunk/lib/Driver/ToolChains/Cuda.h
  cfe/trunk/lib/Frontend/CompilerInvocation.cpp
  cfe/trunk/lib/Serialization/ASTReader.cpp
  cfe/trunk/lib/Serialization/ASTWriter.cpp
  lldb/trunk/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
  llvm/trunk/include/llvm/Support/VersionTuple.h
  llvm/trunk/lib/Support/CMakeLists.txt
  llvm/trunk/lib/Support/VersionTuple.cpp
  llvm/trunk/unittests/Support/CMakeLists.txt
  llvm/trunk/unittests/Support/VersionTupleTest.cpp

Index: llvm/trunk/lib/Support/VersionTuple.cpp
===
--- llvm/trunk/lib/Support/VersionTuple.cpp
+++ llvm/trunk/lib/Support/VersionTuple.cpp
@@ -0,0 +1,110 @@
+//===- VersionTuple.cpp - Version Number Handling ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// This file implements the VersionTuple class, which represents a version in
+// the form major[.minor[.subminor]].
+//
+//===--===//
+#include "llvm/Support/VersionTuple.h"
+#include "llvm/Support/raw_ostream.h"
+
+using namespace llvm;
+
+std::string VersionTuple::getAsString() const {
+  std::string Result;
+  {
+llvm::raw_string_ostream Out(Result);
+Out << *this;
+  }
+  return Result;
+}
+
+raw_ostream &llvm::operator<<(raw_ostream &Out, const VersionTuple &V) {
+  Out << V.getMajor();
+  if (Optional Minor = V.getMinor())
+Out << '.' << *Minor;
+  if (Optional Subminor = V.getSubminor())
+Out << '.' << *Subminor;
+  if (Optional Build = V.getBuild())
+Out << '.' << *Build;
+  return Out;
+}
+
+static bool parseInt(StringRef &input, unsigned &value) {
+  assert(value == 0);
+  if (input.empty())
+return true;
+
+  char next = input[0];
+  input = input.substr(1);
+  if (next < '0' || next > '9')
+return true;
+  value = (unsigned)(next - '0');
+
+  while (!input.empty()) {
+next = input[0];
+if (next < '0' || next > '9')
+  return false;
+input = input.substr(1);
+value = value * 10 + (unsigned)(next - '0');
+  }
+
+  return false;
+}
+
+bool VersionTuple::tryParse(StringRef input) {
+  unsigned major = 0, minor = 0, micro = 0, build = 0;
+
+  // Parse the major version, [0-9]+
+  if (parseInt(input, major))
+return true;
+
+  if (input.empty()) {
+*this = VersionTuple(major);
+return false;
+  }
+
+  // If we're not done, parse the minor version, \.[0-9]+
+  if (input[0] != '.')
+return true;
+  input = input.substr(1);
+  if (parseInt(input, minor))
+return true;
+
+  if (input.empty()) {
+*this = VersionTuple(major, minor);
+return false;
+  }
+
+  // If we're not done, parse the micro version, \.[0-9]+
+  if (input[0] != '.')
+return true;
+  input = input.substr(1);
+  if (parseInt(input, micro))
+return true;
+
+  if (input.empty()) {
+*this = VersionTuple(major, minor, micro);
+return false;
+  }
+
+  // If we're not done, parse the micro version, \.[0-9]+
+  if (input[0] != '.')
+return true;
+  input = input.substr(1);
+  if (parseInt(input, build))
+return true;
+
+  // If we have characters left over, it's an error.
+  if (!input.empty())
+return true;
+
+  *this = VersionTuple(major, minor, micro, build);
+  return false;
+}
Index: llvm/trunk/lib/Support/CMakeLists.txt
===
--- llvm/trunk/lib/Support/CMakeLists.txt
+++ llvm/trunk/lib/Support/CMakeLists.txt
@@ -124,6 +124,7 @@
   Twine.cpp
   Unicode.cpp
   UnicodeCaseFold.cpp
+  VersionTuple.cpp
   WithColor.cpp
   YAMLParser.cpp
   YAMLTraits.cpp
Index

[PATCH] D47886: Move VersionTuple from clang/Basic to llvm/Support, clang part

[Pavel Labath via Phabricator via cfe-commits]

labath closed this revision.
labath added a comment.

Committed as a part of https://reviews.llvm.org/D47887 via monorepo.


Repository:
  rC Clang

https://reviews.llvm.org/D47886



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

[PATCH] D48241: [DebugInfo] Emit ObjC methods as part of interface.

[Pavel Labath via Phabricator via cfe-commits]

labath added a comment.

I also don't want to get involved too much, but here are my 2c: :)

In https://reviews.llvm.org/D48241#1134342, @JDevlieghere wrote:

> Putting it in a separate column is also a bad idea, because that means the 
> column is present for all the entries, including the ones that don't need it.


This is not true. (Unlike the .apple_*** tables, ) .debug_names allows you to 
specify a different schema for every entry in the acelerator table. The schema 
is specifing using abbreviations in much the same way as .debug_abbrev 
specifies the schema for .debug_info. So you could easily have one abbreviation 
for regular classes, and a different one for objc interfaces. Currently, our 
.debug_names generator does not support these kinds of heterogeneous tables, 
but that's simply because I had no use for it. It could be added if necessary 
(though it will require some generalization/refactoring). OTOH, our consumer 
should already be able to handle these kinds of tables as input.

That said, I like this approach more than the putting that information in the 
accel table. :)




Comment at: clang/test/CodeGenObjC/debug-info-category.m:35-37
+// DWARF5: !DISubprogram(name: "-[Foo integer]", scope: ![[STRUCT]], 
{{.*}}isDefinition: false
+// DWARF5: !DISubprogram(name: "-[Foo integer:]", scope: ![[STRUCT]], 
{{.*}}isDefinition: false
+// DWARF5: !DISubprogram(name: "-[Foo(Bar) add:]", scope: ![[STRUCT]], 
{{.*}}isDefinition: false

Would it make sense to remove the interface part from the method name. When 
these were freestanding, they were necessary as they were the only link between 
the method and the interface, but now they are kind of redundant.

This is just an idea, I have no idea what kinds of havoc it will cause for 
existing consumers, but it seems to me it would make the objc methods more 
consistent with regular c++ methods.


https://reviews.llvm.org/D48241



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

[PATCH] D56599: [Support] Remove error return value from one overload of fs::make_absolute

[Pavel Labath via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC351317: [Support] Remove error return value from one 
overload of fs::make_absolute (authored by labath, committed by ).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D56599?vs=181273&id=181997#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D56599

Files:
  lib/Lex/HeaderSearch.cpp


Index: lib/Lex/HeaderSearch.cpp
===
--- lib/Lex/HeaderSearch.cpp
+++ lib/Lex/HeaderSearch.cpp
@@ -1678,9 +1678,8 @@
 StringRef Dir = SearchDirs[I].getDir()->getName();
 llvm::SmallString<32> DirPath(Dir.begin(), Dir.end());
 if (!WorkingDir.empty() && !path::is_absolute(Dir)) {
-  auto err = fs::make_absolute(WorkingDir, DirPath);
-  if (!err)
-path::remove_dots(DirPath, /*remove_dot_dot=*/true);
+  fs::make_absolute(WorkingDir, DirPath);
+  path::remove_dots(DirPath, /*remove_dot_dot=*/true);
   Dir = DirPath;
 }
 for (auto NI = path::begin(File), NE = path::end(File),


Index: lib/Lex/HeaderSearch.cpp
===
--- lib/Lex/HeaderSearch.cpp
+++ lib/Lex/HeaderSearch.cpp
@@ -1678,9 +1678,8 @@
 StringRef Dir = SearchDirs[I].getDir()->getName();
 llvm::SmallString<32> DirPath(Dir.begin(), Dir.end());
 if (!WorkingDir.empty() && !path::is_absolute(Dir)) {
-  auto err = fs::make_absolute(WorkingDir, DirPath);
-  if (!err)
-path::remove_dots(DirPath, /*remove_dot_dot=*/true);
+  fs::make_absolute(WorkingDir, DirPath);
+  path::remove_dots(DirPath, /*remove_dot_dot=*/true);
   Dir = DirPath;
 }
 for (auto NI = path::begin(File), NE = path::end(File),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57330: Adjust documentation for git migration.

[Pavel Labath via Phabricator via cfe-commits]

labath added a comment.

I am not sure we should be recommending to people to place the build folder 
under the llvm-project checkout. Is that how people use the monorepo build 
nowadays? This is not an full out-of-source build, since the build folder is 
still a subfolder of the repo root (and without a .gitignore file containing 
the right build folder name, git will complain about untracked files in the 
repository)...




Comment at: libcxxabi/www/index.html:86
   mkdir build && cd build
-  cmake .. # on linux you may need to prefix with CC=clang 
CXX=clang++
+  cmake -DLLVM_ENABLE_PROJECTS=libcxxabi ../llvm # on linux you may 
need to prefix with CC=clang CXX=clang++
   make

mehdi_amini wrote:
> Do you now if prefixing with CC is equivalent to -DCMAKE_C_COMPILER?
It usually is, but it can differ once you start using cache and toolchain 
files. In any case, using -DCMAKE_C(XX)_COMPILER is the preferred way to do 
things in cmake.


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

https://reviews.llvm.org/D57330



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

[PATCH] D65986: Allow setting the VFS to 'real' mode instead of default 'physical'

[Pavel Labath via Phabricator via cfe-commits]

labath added inline comments.



Comment at: clang/test/Driver/vfsmode.py:4
+
+# UNSUPPORTED: system-windows
+

jfb wrote:
> I'm not sure what the best way to test this on Windows would be, and without 
> a machine handy I can't really test this :-(
Unsupported might be actually correct here. I am not an expert on windows, but 
I have a vague recollection that there, the platform's maximum path limit 
applies to the final absolute path of the file, irrespective of how you happen 
to refer to that file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65986



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

[PATCH] D51641: [VFS] Cache the current working directory for the real FS.

[Pavel Labath via Phabricator via cfe-commits]

labath added subscribers: JDevlieghere, labath.
labath added a comment.

Might I ask what was the motivation for this change? Performance optimalization?

I am asking this because this makes the RealFileSystem return bogus values for 
the CWD if it changes through means other than the 
VFS::setCurrentWorkingDirectory (which is something that, as a library, we can 
never rule out). We ran into problems with this in lldb where our tests use 
lldb as a library, and they are driven by a python scripts (which does some CWD 
changing as a part of test discovery).

If I understand the long scary comment in the `setCurrentWorkingDirectory` 
function correctly, the RealFileSystem wants to be independent of the OS notion 
of CWD. However, right now it's not doing either job very well (sharing the CWD 
with the OS nor being independent of it), because all other RFS functions will 
use the real OS CWD (as it is at the moment of the call), only 
`getCurrentWorkingDirectory` will return whatever was the last CWD set through 
the appropriate setter.


Repository:
  rC Clang

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

https://reviews.llvm.org/D51641



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

[PATCH] D51641: [VFS] Cache the current working directory for the real FS.

[Pavel Labath via Phabricator via cfe-commits]

labath added a comment.

I wholeheartedly support an openat(2) based VFS, as the current one falls short 
of the expectations you have of it and is pretty broken right now.

As for your assumption #2, I think that depends on what does one want to get 
out of the VFS. I can certainly see a use for having a  thread-local 
(VFS-local) CWD. In fact I remember seeing some code in LLDB, which used some 
darwin-only interface to create a real thread-local CWD in one function. 
However, I can also see a use for a VFS which is so "real" that it shares the 
notion of the CWD with the OS. What could happen in lldb is that a user sets 
the CWD in python (using the proper python api), and then passes a relative 
path to (lib)lldb. Since lldb never advertised to have any notion of a 
"lldb-local" CWD, the user would (righfully) expect that that path is resolved 
relative to the CWD he has just set. And that's what happened until we started 
using VFS. In fact, it still happens now, because the VFS is so bad at having a 
local CWD. So, the only reason we actually discovered this was because  
VFS->getCurrentWorkingDirectory returned an empty string if a it's CWD has 
never been set. This later translated to a null pointer and a crash.

So it almost sounds to me like we should have two implementations of the VFS. A 
"real" one, which shared the CWD with the OS, and an "almost real" one, which 
has a local CWD. Another position would be to just say that lldb was wrong to 
use the VFS in the first place, as it does not promise we want (and it should 
use a different abstraction instead). That view would be supported by the fact 
that there are other interface disconnects relating to the use of VFS in lldb 
(FILE* and friends). However, that's something you and Jonas will have to 
figure out (I'm just the peanut gallery here :P).


Repository:
  rC Clang

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

https://reviews.llvm.org/D51641



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

[PATCH] D51641: [VFS] Cache the current working directory for the real FS.

[Pavel Labath via Phabricator via cfe-commits]

labath added a comment.

In D51641#1352782 , @sammccall wrote:

> > In fact, it still happens now, because the VFS is so bad at having a local 
> > CWD. So, the only reason we actually discovered this was because  
> > VFS->getCurrentWorkingDirectory returned an empty string if a it's CWD has 
> > never been set. This later translated to a null pointer and a crash.
>
> (Hmm, `RealFileSystem::getCurrentWorkingDirectory shouldn't return empty 
> unless `fs::current_path()` does. But certainly here be dragons)


My bad. After re-reading the original report it seems that happened was that we 
got a stale CWD, then went to search for a file that we expected to be there 
(but it wasn't, because we were searching in the wrong place), and *then* we 
crashed. :)


Repository:
  rC Clang

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

https://reviews.llvm.org/D51641



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

[PATCH] D47532: [ASTImporter] Import the whole redecl chain of functions

[Pavel Labath via Phabricator via cfe-commits]

labath added subscribers: aprantl, labath.
labath added a comment.

This has broken the LLDB bot 
http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-cmake/builds/25132. 
Could you take a look?


Repository:
  rC Clang

https://reviews.llvm.org/D47532



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

[PATCH] D47532: [ASTImporter] Import the whole redecl chain of functions

[Pavel Labath via Phabricator via cfe-commits]

labath added a comment.

In https://reviews.llvm.org/D47532#1144966, @martong wrote:

> The broken lldb tests are fixed with a minor change. We no longer load the 
> Decls from the
>  external source during the call of `DeclContext::containsDecl`.  A new 
> function
>  `DeclContext::containsDeclAndLoad` is added which does a load and calls
>  `containsDecl`.
>
> Re-apply commit: r335731


Thank you.


Repository:
  rC Clang

https://reviews.llvm.org/D47532



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

[PATCH] D46685: [CodeGen] Disable structor optimizations at -O0

[Pavel Labath via Phabricator via cfe-commits]

labath added a comment.

This was reverted in r333482 because it was causing "definition with same 
mangled name as another definition" errors in some internal google builds.

It turned out this uncovered an (unrelated) issue in module importing. This has 
now been fixed (r336240), so I'm planning to resubmit this patch. Let me know 
if you have any concerns.


Repository:
  rC Clang

https://reviews.llvm.org/D46685



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

[PATCH] D84554: Use INTERFACE_COMPILE_OPTIONS to disable -Wsuggest-override for any target that links to gtest

[Pavel Labath via Phabricator via cfe-commits]

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

This looks good to me. Thanks.

Could you elaborate on the lldb issue? I'd like to take a look at that...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84554



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

[PATCH] D84554: Use INTERFACE_COMPILE_OPTIONS to disable -Wsuggest-override for any target that links to gtest

[Pavel Labath via Phabricator via cfe-commits]

labath added a comment.

In D84554#2176041 , @logan-5 wrote:

> In D84554#2175012 , @labath wrote:
>
>> Could you elaborate on the lldb issue? I'd like to take a look at that...
>
> It looks like lldb/unittests/TestingSupport/CMakeLists.txt and 
> lldb/unittests/TestingSupport/Symbol/CMakeLists.txt both both manually pull 
> in the googletest/googlemock headers via `include_directories`. It also 
> seems, although I'm not sure, like they don't themselves link to gtest, since 
> simply removing those `include_directories` didn't work.

Thanks. I think I understand the problem now. Those libraries should be linking 
against gtest, but they don't do that because they could get away with not 
doing it. And they add include directories manually because they do not get 
that from linking against gtest. That means we have more things to fix => 
D84748 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84554

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

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

[Pavel Labath via Phabricator via cfe-commits]

labath added a comment.

I wouldn't mind separate (internal) cache variable, though I am somewhat 
surprised by this problem. A (non-cache) variable set in one of the parent 
scopes should still take precedence over a cache variable with the same name. 
And since config-ix.cmake is included from the top CMakeLists.txt, the value it 
defines should be available everywhere. Was this a problem for the regular 
build, or only for some of the more exotic build setups that don't start with 
llvm/CMakeLists.txt ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79219



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

[PATCH] D76801: [AST] Print a> without extra spaces in C++11 or later.

[Pavel Labath via Phabricator via cfe-commits]

labath added a comment.

David's example does work with gdb without -Wl,--gdb-index (the member variable 
is shown), presumably due to the aforementioned fuzzy matching. However, it 
does not work if gdb-index is enabled, nor with lldb (as lldb is always very 
index-oriented and assumes equality everywhere). That is definitely not ideal, 
though I'm not sure that means about this patch. This is definitely not the 
only difference in the formatting of DW_AT_names of templates. For example, 
`template operator<<(T, T)` will come out as `operator<< ` with 
gcc, but as `operator<<` with clang (with or without this patch).
OTOH, differences in type names are more likely to cause problems than is the 
case for functions/operators.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76801



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

[PATCH] D76801: [AST] Print a> without extra spaces in C++11 or later.

[Pavel Labath via Phabricator via cfe-commits]

labath added a comment.

In D76801#1993337 , @dblaikie wrote:

> In D76801#1991904 , @labath wrote:
>
> > David's example does work with gdb without -Wl,--gdb-index (the member 
> > variable is shown), presumably due to the aforementioned fuzzy matching. 
> > However, it does not work if gdb-index is enabled,  nor with lldb (as lldb 
> > is always very index-oriented and assumes equality everywhere). That is 
> > definitely not ideal, though I'm not sure that means about this patch. This 
> > is definitely not the only difference in the formatting of DW_AT_names of 
> > templates. For example, `template operator<<(T, T)` will come 
> > out as `operator<< ` with gcc, but as `operator<<` with clang (with 
> > or without this patch).
> >  OTOH, differences in type names are more likely to cause problems than is 
> > the case for functions/operators.
>
>
> That is concerning. Any idea if that's only with lld's gdb-indexx 
> implementation, or also gold's?


I was using gold for my initial test. However, the same thing happens with lld 
too...

> This isn't the only naming divergence between GCC and Clang, though, so if 
> gdb-index doesn't support any divergence, that's a problem...

It becomes a gdb-index problem because with an index the debugger will do a 
(hashed?) string lookup and expect the string to be there. If the strings 
differ, the lookup won't find anything. In the no-index scenario, the debugger 
has to trawl the debug info itself, and so it has some opportunities to do 
fuzzy matching.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76801



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

[PATCH] D76801: [AST] Print a> without extra spaces in C++11 or later.

[Pavel Labath via Phabricator via cfe-commits]

labath added a comment.

In D76801#1995058 , @dblaikie wrote:

> > It becomes a gdb-index problem because with an index the debugger will do a 
> > (hashed?) string lookup and expect the string to be there. If the strings 
> > differ, the lookup won't find anything. In the no-index scenario, the 
> > debugger has to trawl the debug info itself, and so it has some 
> > opportunities to do fuzzy matching.
>
> That surprises me a bit - given that one of the things debuggers provide is 
> autocomplete (I'm unlikely to write out a type name exactly as the debug info 
> contains it - if two compilers can't agree, it's much less likely that all 
> users will agree with any compiler rendering), which I'd have thought would 
> be facilitated by the index too - in which case lookup via exact match 
> wouldn't be viable, you'd still want a way to list anything with a matching 
> substring (or at least prefix), etc. Which could help facilitate lookup fuzzy 
> matching like in this sort of case.


That is kind of true, and I don't really have a definitive reply to that. I 
suppose there is a difference between the lookups done for type completion and 
those done e.g. in expression evaluation. The latter are probably more frequent 
and are assumed to be correct. Maybe they shouldn't be, but in that cases, then 
there would probably be no use for the hash tables in indexes (I am not very 
familiar with the gdb index, but I know it has hash tables similar to 
debug_names/apple_names). I don't know what gdb does for tab completion, but it 
probably bypasses the hash table (though the index could still be useful even 
then as it has a concise list of all strings in the debug info), or it gets the 
list of strings-to-complete from a completely different source (demangled 
function names?).

Tab completion is always a bit dodgy. E.g., in your example `ptype tmpl` 
completes to `ptype tmpl>`, but then running that produces an error: 
`No symbol "tmpl" in current context.`

In lldb, tab completion in expressions works by hooking into the regular clang 
tab-completion machinery used by editors.  The up- and down-side of that is 
that it uses the same code path used for actual expression evaluation -- i.e. 
all the types will be looked up the same (exact) way.

Speaking of templates and indexes, the thing we would really like in lldb would 
be to have just the bare names of templated class in the indexes -- that way we 
could reliably look up all instantiations of a template and do the filtering 
ourselves. However, this runs afoul of the dwarf specification, which says that 
the index names should match the DW_AT_names of relevant DIEs (and these 
contain the template arguments for other reasons...). This means that currently 
we have outstanding issues when looking up templated types, but we haven't 
really figured out what to do about that...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76801



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

[PATCH] D76801: [AST] Print a> without extra spaces in C++11 or later.

[Pavel Labath via Phabricator via cfe-commits]

labath added a comment.

In D76801#1997451 , @dblaikie wrote:

> Yeah, points all taken - as for this actual issue... I'm kind of inclined to 
> say "hey, our template names already diverge somewhat - and this divergence 
> is in the realm of acceptable by gdb (without an index) so... *thumbs 
> up*/let's stick with it"


Another interesting aspect here is that the DW_AT_name outputs depend on the 
c++ standard versions used. This means we could get mismatches even with the 
same compiler if some compile units use `-std=c++98`, and others `-std>=c++11` 
(hardly a recommended practice but it does work if one knows what he is doing). 
Compatibility with another compiler is one thing, but maybe self-compatibility 
is more important (and easier to achieve) ?

One way to  achieve that would be by printing all type names in c++98 mode, 
which (IIUC) is the same thing as what the windows folks are requesting..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76801



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

[PATCH] D79219: [CMake] Simplify CMake handling for zlib

[Pavel Labath via Phabricator via cfe-commits]

labath added inline comments.



Comment at: llvm/cmake/config-ix.cmake:514
+  if(ZLIB_FOUND)
+set(LLVM_ENABLE_ZLIB "YES" CACHE STRING
+  "Use zlib for compression/decompression if available. Can be ON, OFF, or 
FORCE_ON"

JDevlieghere wrote:
> phosek wrote:
> > JDevlieghere wrote:
> > > If I understand correctly, after running the configuration phase with 
> > > LLVM_ENABLE_ZLIB to FORCE_ON, the cached value will be overwritten by ON? 
> > Correct, we used `FORCE_ON` above when invoking `find_package` setting 
> > `REQUIRED` which makes the check fail if the library is missing. Recording 
> > this information is important for LLVM consumers because it'll be stored in 
> > `LLVMConfig.cmake` and AFAIU we don't want to propagate `FORCE_ON` there.
> I get that. My worry is that if for whatever reason the library disappears 
> (system upgrade, package removal, ...) this will silently disable ZLIB 
> support because now LLVM_ENABLE_ZLIB just equals on. This might sound far 
> fetched, but happens all the time with "internal installs". This is why I 
> prefer the ON/OFF/Auto approach because it doesn't update the cache variable, 
> but would set the internal variable to either ON or OFF.
I agree with Jonas, though I don't think the actual values (FORCE_ON/ON vs. 
ON/Auto) really matter here. The important part is not overwriting the cache 
variables specified by the user, as that can create various problems with 
reconfigurations (like the zlib becoming unavailable example).


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

https://reviews.llvm.org/D79219



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

[PATCH] D80001: [RFC/WIP][clang] Fix printing of names of inherited constructors

[Pavel Labath via Phabricator via cfe-commits]

labath created this revision.
labath added reviewers: rsmith, dblaikie.
Herald added a subscriber: aprantl.
Herald added a project: clang.

This is a fairly hacky fix to the following problem: Debug information
entries for inherited constructors are emitted with the name of the base
class, instead of the inheriting class.

This problem can be tracked down to a FIXME in
Sema::findInheritingConstructor, which works around the problem of not
having a DeclarationName to describe inherited constructors. The current
patch compounds that workaround by printing the inherited constructor
name in a different way.

I don't really expect this patch to make its way into the tree in the
current form. I am mainly putting it up to point out the problem and
start a discussion on the solution. I suppose the right fix is to
implement the FIXME, and add a new DeclarationName kind. But that seems
like it could be a fairly involved change, so I wanted to get some
confirmation (and maybe a bit of guidance) first.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D80001

Files:
  clang/include/clang/AST/DeclCXX.h
  clang/test/CodeGenCXX/debug-info-inheriting-constructor.cpp
  clang/test/CodeGenCXX/debug-info-inlined.cpp


Index: clang/test/CodeGenCXX/debug-info-inlined.cpp
===
--- clang/test/CodeGenCXX/debug-info-inlined.cpp
+++ clang/test/CodeGenCXX/debug-info-inlined.cpp
@@ -26,4 +26,4 @@
 // CHECK-SAME: !dbg ![[INL:[0-9]+]]
 
 // CHECK: ![[INL]] = !DILocation(line: 10, scope: ![[SP:[0-9]+]], inlinedAt:
-// CHECK: ![[SP]] = distinct !DISubprogram(name: "Base", {{.*}} 
DISPFlagDefinition
+// CHECK: ![[SP]] = distinct !DISubprogram(name: "Forward", {{.*}} 
DISPFlagDefinition
Index: clang/test/CodeGenCXX/debug-info-inheriting-constructor.cpp
===
--- clang/test/CodeGenCXX/debug-info-inheriting-constructor.cpp
+++ clang/test/CodeGenCXX/debug-info-inheriting-constructor.cpp
@@ -16,10 +16,10 @@
 // CHECK-SAME: metadata ![[THIS:[0-9]+]], metadata !DIExpression()), !dbg 
![[LOC:[0-9]+]]
 // CHECK: ret void, !dbg ![[NOINL:[0-9]+]]
 // CHECK: ![[FOO:.*]] = distinct !DISubprogram(name: "foo"
-// CHECK-DAG: ![[A:.*]] = distinct !DISubprogram(name: "A", linkageName: 
"_ZN1BCI11AEiz"
+// CHECK-DAG: ![[B:.*]] = distinct !DISubprogram(name: "B", linkageName: 
"_ZN1BCI11AEiz"
 void foo() {
-// CHECK-DAG: ![[LOC]] = !DILocation(line: 0, scope: ![[A]], inlinedAt: 
![[INL:[0-9]+]])
-// CHECK-DAG: ![[INL]] = !DILocation(line: [[@LINE+1]], scope: ![[FOO]])
+  // CHECK-DAG: ![[LOC]] = !DILocation(line: 0, scope: ![[B]], inlinedAt: 
![[INL:[0-9]+]])
+  // CHECK-DAG: ![[INL]] = !DILocation(line: [[@LINE+1]], scope: ![[FOO]])
   B b(0);
 // CHECK: ![[NOINL]] = !DILocation(line: [[@LINE+1]], scope: !{{[0-9]+}})
 }
Index: clang/include/clang/AST/DeclCXX.h
===
--- clang/include/clang/AST/DeclCXX.h
+++ clang/include/clang/AST/DeclCXX.h
@@ -2631,6 +2631,13 @@
 return const_cast(this)->getCanonicalDecl();
   }
 
+  void printName(raw_ostream &os) const override {
+if (isInheritingConstructor())
+  getParent()->printName(os);
+else
+  CXXMethodDecl::printName(os);
+  }
+
   // Implement isa/cast/dyncast/etc.
   static bool classof(const Decl *D) { return classofKind(D->getKind()); }
   static bool classofKind(Kind K) { return K == CXXConstructor; }


Index: clang/test/CodeGenCXX/debug-info-inlined.cpp
===
--- clang/test/CodeGenCXX/debug-info-inlined.cpp
+++ clang/test/CodeGenCXX/debug-info-inlined.cpp
@@ -26,4 +26,4 @@
 // CHECK-SAME: !dbg ![[INL:[0-9]+]]
 
 // CHECK: ![[INL]] = !DILocation(line: 10, scope: ![[SP:[0-9]+]], inlinedAt:
-// CHECK: ![[SP]] = distinct !DISubprogram(name: "Base", {{.*}} DISPFlagDefinition
+// CHECK: ![[SP]] = distinct !DISubprogram(name: "Forward", {{.*}} DISPFlagDefinition
Index: clang/test/CodeGenCXX/debug-info-inheriting-constructor.cpp
===
--- clang/test/CodeGenCXX/debug-info-inheriting-constructor.cpp
+++ clang/test/CodeGenCXX/debug-info-inheriting-constructor.cpp
@@ -16,10 +16,10 @@
 // CHECK-SAME: metadata ![[THIS:[0-9]+]], metadata !DIExpression()), !dbg ![[LOC:[0-9]+]]
 // CHECK: ret void, !dbg ![[NOINL:[0-9]+]]
 // CHECK: ![[FOO:.*]] = distinct !DISubprogram(name: "foo"
-// CHECK-DAG: ![[A:.*]] = distinct !DISubprogram(name: "A", linkageName: "_ZN1BCI11AEiz"
+// CHECK-DAG: ![[B:.*]] = distinct !DISubprogram(name: "B", linkageName: "_ZN1BCI11AEiz"
 void foo() {
-// CHECK-DAG: ![[LOC]] = !DILocation(line: 0, scope: ![[A]], inlinedAt: ![[INL:[0-9]+]])
-// CHECK-DAG: ![[INL]] = !DILocation(line: [[@LINE+1]], scope: ![[FOO]])
+  // CHECK-DAG: ![[LOC]] = !DILocation(line: 0, scope: ![[B]], inlinedAt: ![[INL:[0-9]+]])
+  // CHECK-DAG: ![[INL]] = !DILocation(line: [[@LINE+1]], scope

[PATCH] D131007: [NFCI] Refactor how KeywordStatus is calculated

[Pavel Labath via Phabricator via cfe-commits]

labath added a comment.

This seems to break lldb tests, specifically the compilation of this 

 source file.

The compile fails with an assertion (`Keyword not known to come from a newer 
Standard or proposed Standard`), so I am assuming that change was not 
intentional.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131007

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

[PATCH] D119910: [clang][debug] port clang-cl /JMC flag to ELF

[Pavel Labath via Phabricator via cfe-commits]

labath added a comment.

Not my usual wheelhouse, but since I'm here, here are some of my thoughts:

- reusing the existing solution/code seems like a good idea -- no need to 
reinvent the wheel
- how does that work, actually? How does it differentiate between "my" code and 
"code that happens to be compiled in the current CU" (e.g. the STL).  What's 
the significance of the funky `___x@c` symbols?
- if the goal is to integrate this into lldb (and given that lldb currently 
does not support any feature like this), I'd recommend sending an RFC/heads up 
early, to identify possible issues


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119910

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

[PATCH] D124606: Use `binary` git attribute instead of `text eol=...'

[Pavel Labath via Phabricator via cfe-commits]

labath added a comment.

I believe (based on some similar issues I was solving in lldb) that `-text` 
instead of `binary` is sufficient to prevent git from fidlling with the 
contents, while maintaining the ability to diff the files.

Other than that, I am in favour of this approach, but I'd recommend getting 
this reviewed by someone on the clang-tools team.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124606

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

[PATCH] D124606: Use `-text` git attribute instead of `text eol=...'

[Pavel Labath via Phabricator via cfe-commits]

labath added inline comments.



Comment at: clang-tools-extra/test/.gitattributes:7-15
+clang-apply-replacements/ClangRenameClassReplacements.cpp -text
+clang-apply-replacements/Inputs/basic/basic.h -text
+clang-apply-replacements/Inputs/format/no.cpp -text
+clang-apply-replacements/Inputs/format/yes.cpp -text
+clang-tidy/infrastructure/export-diagnostics.cpp -text
 
 # These test input files rely on two-byte Windows (CRLF) line endings.

aaronpuchert wrote:
> My understanding is that `-text` removes an attribute, but what attribute is 
> there to remove? There is no global `.gitattributes` that would set it in the 
> first place. This change just reverts to the status quo before D97625, or 
> rather before that file was moved.
Are you sure about that? Git seems to distinguish between "unset" and 
"unspecified" values of an attribute:
```
   Each attribute can be in one of these states for a given path:

   Set
   The path has the attribute with special value "true"; this is 
specified by listing only the
   name of the attribute in the attribute list.

   Unset
   The path has the attribute with special value "false"; this is 
specified by listing the
   name of the attribute prefixed with a dash - in the attribute list.

   Set to a value
   The path has the attribute with specified string value; this is 
specified by listing the
   name of the attribute followed by an equal sign = and its value in 
the attribute list.

   Unspecified
   No pattern matches the path, and nothing says if the path has or 
does not have the
   attribute, the attribute for the path is said to be Unspecified.

```

And then it says this about the `text` attribute in particular:
```
   Set
   Setting the text attribute on a path enables end-of-line 
normalization and marks the
   path as a text file. End-of-line conversion takes place without 
guessing the content
   type.

   Unset
   Unsetting the text attribute on a path tells Git not to attempt 
any end-of-line
   conversion upon checkin or checkout.

   Set to string value "auto"
   When text is set to "auto", the path is marked for automatic 
end-of-line conversion. If
   Git decides that the content is text, its line endings are 
converted to LF on checkin.
   When the file has been committed with CRLF, no conversion is 
done.

   Unspecified
   If the text attribute is unspecified, Git uses the core.autocrlf 
configuration variable
   to determine if the file should be converted.

```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124606

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

[PATCH] D124606: Use `-text` git attribute instead of `text eol=...'

[Pavel Labath via Phabricator via cfe-commits]

labath added a comment.

In D124606#3480164 , @aaronpuchert 
wrote:

> Fair enough, but don't we want to enforce LF or CRLF, respectively?

Sure, but is the version control system the right tool to do that? I think it'd 
be better to have the test itself confirm the consistency of the data (or 
convert it into the right format at runtime) and not rely on magical 
conversions within the VC tool.

> An editor could inadvertently change the line endings, and someone might not 
> notice before committing.

From what I understood, most of these tests would immediately break (due to 
hardcoded offsets) if the line endings were changed.

In D124606#3480187 , @aaronpuchert 
wrote:

> Also I still don't understand what specifically this is fixing. What exactly 
> was wrong about the previous configuration?

The branch switching issue (discussed in D124563 
) is one. We also ran into problems when 
importing this into our version control system (as it did not do the 
.gitattributes conversion) -- although one could say that is not an upstream 
problem...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124606

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

[PATCH] D59692: [ASTImporter] Fix name conflict handling with different strategies

[Pavel Labath via Phabricator via cfe-commits]

labath added a comment.

In D59692#1646990 , @martong wrote:

> Jenkins is not green 
> http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/743/
>  However, the newly failing test is `TestTargetCommand.py`, which seems to be 
> unrelated.


That ought to be fixed by r370057.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59692



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

[PATCH] D87590: Backport D79219, D85820, D86134 to 10.0 branch

[Pavel Labath via Phabricator via cfe-commits]

labath added subscribers: tstellar, labath.
labath added a reviewer: tstellar.
labath added a comment.

@tstellar, who is (was) the release manager for 10.0, can make a definitive 
call, but our usual modus operandi is to have one point release for each major 
version. I don't know if this is a sufficiently major issue to break from that 
(and it definitely is risky).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87590

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

[PATCH] D87590: Backport D79219, D85820, D86134 to 10.0 branch

[Pavel Labath via Phabricator via cfe-commits]

labath added a comment.

In D87590#2273721 , @haampie wrote:

> Also note that the changes are not likely to be backported to 11 even: 
> https://reviews.llvm.org/rG31e5f7120bdd2f76337686d9d169b1c00e6ee69c#942622.

11.0.0, probably. There's should be enough time for 11.0.1, though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87590

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

[PATCH] D86660: Modifying ImportDeclContext(...) to ensure that we also handle the case when the FieldDecl is an ArrayType whose ElementType is a RecordDecl

[Pavel Labath via Phabricator via cfe-commits]

labath added inline comments.



Comment at: clang/lib/AST/ASTImporter.cpp:1755-1759
+  QualType FromTy = ArrayFrom->getElementType();
+  QualType ToTy = ArrayTo->getElementType();
+
+  FromRecordDecl = FromTy->getAsRecordDecl();
+  ToRecordDecl = ToTy->getAsRecordDecl();

What about 2- or n-dimensional arrays?



Comment at: 
lldb/test/API/commands/expression/codegen-crash-import-def-arraytype-element/main.cpp:22-27
+  union {
+struct {
+  unsigned char *_s;
+} t;
+char *tt[1];
+  } U;

What's the significance of this union?


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

https://reviews.llvm.org/D86660

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

[PATCH] D86660: Modifying ImportDeclContext(...) to ensure that we also handle the case when the FieldDecl is an ArrayType whose ElementType is a RecordDecl

[Pavel Labath via Phabricator via cfe-commits]

labath added inline comments.



Comment at: clang/lib/AST/ASTImporter.cpp:1755-1759
+  QualType FromTy = ArrayFrom->getElementType();
+  QualType ToTy = ArrayTo->getElementType();
+
+  FromRecordDecl = FromTy->getAsRecordDecl();
+  ToRecordDecl = ToTy->getAsRecordDecl();

shafik wrote:
> martong wrote:
> > labath wrote:
> > > What about 2- or n-dimensional arrays?
> > @labath, this is a very good question! And made me realize that D71378 is 
> > fundamentally flawed (@shafik, please take no offense). Let me explain.
> > 
> > So, with D71378, we added the `if (ImportedOrErr) { ... }` block to import 
> > definitions specifically of fields' Record types. But we forget to handle 
> > arrays. Now we may forget to handle multidimensional arrays ... and we may 
> > forget to handle other language constructs. So, we would finally end up in 
> > re-implementing the logic of `ASTNodeImporter::VisitFieldDecl`.
> > 
> > So all this should have been handled properly by the preceding import call 
> > of the FieldDecl! Here
> > ```
> > 1735: ExpectedDecl ImportedOrErr = import(From);
> > ```
> > I have a suspicion that real reason why this import call fails in case of 
> > the public ASTImporter::ImportDefinition() is that we fail to drive through 
> > the import kind (`IDK_Everything`) during the import process.
> > Below we set IDK_Everything and start a complete import process.
> > ```
> >   8784   if (auto *ToRecord = dyn_cast(To)) {
> >   8785 if (!ToRecord->getDefinition()) {
> >   8786   return Importer.ImportDefinition(   // 
> > ASTNodeImporter::ImportDefinition !
> >   8787   cast(FromDC), ToRecord,
> >   8788   ASTNodeImporter::IDK_Everything);
> >   8789 }
> >   8790   }
> > ```
> > However, there may be many places where we fail to channel through that we 
> > want to do a complete import. E.g here
> > ```
> > 1957   
> > ImportDefinitionIfNeeded(Base1.getType()->getAsCXXRecordDecl()))
> > ```
> > we do another definition import and IDK_Everything is not set. So we may 
> > have a wrong kind of import since the "minimal" flag is set.
> > 
> > The thing is, it is really confusing and error-prone to have both the 
> > `ASTImporter::Minimal` flag and the `ImportDefinitionKind`. They seem to be 
> > in contradiction to each other some times.
> > I think we should get rid of the Minimal flag. And Kind should be either a 
> > full completion (IDK_Everythink) or just a minimal (IDK_Basic). So, I'd 
> > scrap the IDK_Default too. Alternatively we could have a Kind member in 
> > AST**//Node//**Importer.
> > I think we should go into this direction to avoid similar problems during 
> > CodeGen in the future. WDYT?
> No offense, you definitely understand the Importer better than I, so I value 
> your input here. I don't believe that should have other fields where we could 
> have a record that effects the layout but the concern is still valid and yes 
> I did miss multi-dimensional arrays.
> 
> Your theory on `IDK_Everything` not be pushed through everywhere, I did a 
> quick look and it seems pretty reasonable. 
> 
> I agree that the `ASTImporter::Minimal` flag and the `ImportDefinitionKind` 
> seem to inconsistent or perhaps a work-around. That seems like a bigger 
> change with a lot more impact beyond this fix but worth looking into longer 
> term. 
> 
> If pushing through `IDK_Everything` everywhere fixes the underlying issue I 
> am very happy to take that approach. If not we can discuss alternatives. 
I've been looking at this code, but I'm still not very familiar with it, so 
what I am asking may be obvious, but... What is the expected behavior for 
non-minimal imports for code like this?
```
struct A { ...};
struct B { A* a; }; // Note the pointer
```
Should importing B also import the definition of the A struct ? As I think that 
should not happen in the minimal import... If we get rid of the minimal flag, 
and rely solely on argument passing, we will need to be careful, as it 
shouldn't be passed _everywhere_ (it should stop at pointers for instance). But 
then it may not be possible to reproduce the current non-minimal import, as it 
(I think) expects that A will be fully imported too...

> I don't believe that should have other fields where we could have a record 
> that effects the layout
This isn't exactly layout related, but there is the question of covariant 
methods. If a method is covariant, then its return type must be complete. 
Currently we handle the completion of these in LLDB, but that solution is a bit 
hacky. It might be interesting if that could be handled by the ast importer as 
well.



Comment at: 
lldb/test/API/commands/expression/codegen-crash-import-def-arraytype-element/main.cpp:22-27
+  union {
+struct {
+  unsigned char *_s;
+} t;
+char *tt[1];
+  } U;

shafik wrote:
> labath wrote:
> > What's the significance of this union?
> Not sur

[PATCH] D86660: Modifying ImportDeclContext(...) to ensure that we also handle the case when the FieldDecl is an ArrayType whose ElementType is a RecordDecl

[Pavel Labath via Phabricator via cfe-commits]

labath added a comment.

I'm going to respond to the rest of your (very insightful) comment later. So 
far, I'm just responding to this:

>>   This isn't exactly layout related, but there is the question of covariant 
>> methods. If a method is covariant, then its return type must be complete.
>
> We already import all the base classes of a derived class (at least in full 
> import). But, we do not import all the derived classes during the import of 
> the base. Is this what you do in LLDB to support covariant return types?

This situation is a bit tricky to explain as there are two independent class 
hierarchies that need to be considered. Let me start with an example:

  struct B1;
  struct B2;
  struct A1 { B1 *f(); };
  struct A2 { B2 *f(); };

This is a perfectly valid C++ snippet. In fact it could be extended to also 
implement `A1::f` and `A2::f` and call them and it would still not require the 
definitions for structs `B1` and `B2`. And I believe the ast importer will 
would not import their definitions even if they were available. It certainly 
wouldn't force them to be defined (`getExternalSource()->CompleteType(B1)`).

This changes if the methods become virtual. In this case, this code becomes 
valid only if `B2*` is convertible to `B1*` (i.e., `B2` is derived from `B1`). 
To know that, both `B1` and `B2` have to be complete. Regular clang parser will 
enforce that, and codegen will depend on it. However, the AST importer will not 
automatically import these classes. Lldb's code to do that is here 
https://github.com/llvm/llvm-project/blob/fdc6aea3fd822b639baaa5b666fdf7598d08c8de/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp#L994.

I don't know whether something like this would be in scope for the default ast 
importer, but it seemed useful to mention in the context of the present 
discussion.


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

https://reviews.llvm.org/D86660

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

[PATCH] D70764: build: reduce CMake handling for zlib

[Pavel Labath via Phabricator via cfe-commits]

labath added a comment.

Please take a look at D70519  for the issues 
with this approach.

Also, while I do agree with you that we should not auto-select dependencies, I 
think this runs contrary to the llvm philosophy that the default built should 
"just work" (I don't know if that's formalized anywhere, but I know it's 
certainly what (some?) people expect).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70764



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

[PATCH] D70764: build: reduce CMake handling for zlib

[Pavel Labath via Phabricator via cfe-commits]

labath added a comment.

`/usr/lib/x86_64-linux-gnu/libz.so` is definitely better than 
`-l/usr/lib/x86_64-linux-gnu/libz.so`, as it's at least a valid link line. It's 
not completely equivalent, and may not work in all cases -- last time I 
accidentally changed this, I got about a dozen emails from people I broke, and 
I wouldn't be surprised if one of them depended on the subtle differences here. 
However, the fact that libxml is also spelled out this way gives me some hope...

That said, this still leaves us with the "problem" that the default cmake 
configuration will be broken for people who don't have zlib installed (pretty 
much everyone on windows, at least). You can try that out by 
deleting/renaming/uninstalling zlib.h from your system. With this patch you get 
a configuration error whereas previously zlib support was automatically 
disabled. Whether this "problem" is a **problem** or "working as intended" 
depends on what do you expect out of your build system, but it's definitely not 
consistent with how other llvm dependencies are managed.

Anyway, I don't really have a horse in this race, but I figured I should at 
least warn you about the problems you're likely to run into. (and I really wish 
@beanz would say something here, as he's the one who's complaining about build 
system inconsistencies...)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70764



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

[PATCH] D71508: [DebugInfo] Duplicate file names in debug info

[Pavel Labath via Phabricator via cfe-commits]

labath added a comment.

In D71508#1785767 , @probinson wrote:

> Do we have a similar problem if the filespec has an embedded ./ or ../ in it? 
>  I'm thinking some broader canonicalization ought to be done here.
>  $ clang ./dir1/dir2/../dir3/file.c
>  should resolve to dir1/dir3/file.c shouldn't it?


I would be very careful about aggressive canonicalization. Once you throw 
symlinks into the mix, there's very little things you can safely do. If `dir2` 
is a symlink then `dir2/..` can point pretty much anywhere. And if you use 
something like `realpath` there's no telling whether the final result actually 
be the thing you actually want to put into the debug info (your whole source 
tree could be a "symlink farm" with individual files pointing to random 
unpredictable locations).

It seems to me that the underlying problem here is that there are different 
kinds of canonicalization applied to the "main" and other files, which is 
something that the comment in CDDebugInfo::CreateCompileUnit seems to 
acknowledge. However, I don't know anything about this code to say how/if it is 
possible to fix that...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71508



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

[PATCH] D71707: clang-tidy: new bugprone-pointer-cast-widening

[Pavel Labath via Phabricator via cfe-commits]

labath resigned from this revision.
labath added a comment.

Though this *was* my idea, I don't really feel qualified to review code here.

However, some things to consider:

- disallowing casts to intptr_t seems too restrictive -- I doubt many people 
are doing that, but I guess this type exists for a reason, and since the type 
(and it's signedness) is spelled out in the source, it shouldn't be too 
surprising that sign-extension can happen later
- requiring a literal uintptr_t (or a typedef of it) may be also problematic -- 
the user could obtain an integer type of the same bit width through some other 
means (e.g. `#ifdef`). OTOH, without that (and just checking the bit width for 
instance), one would have to actually compile for a 32-bit target to get this 
warning. I don't know what's the practice for this in clang-tidy...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71707



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

[PATCH] D144181: [clang][DebugInfo] Add abi-tags on constructors/destructors as LLVM annotations

[Pavel Labath via Phabricator via cfe-commits]

labath added a comment.

In D144181#4142295 , @Michael137 
wrote:

> We thought a bit about what it would take to link a constructor declaration 
> DIE to the various definitions (e.g., via a 
> `DW_AT_LLVM_complete_ctor_linkage_name` or `DW_AT_LLVM_complete_ctor_ref`). 
> The issue with this is that it would mess with type uniquing. E.g., what if 
> two different CUs each had a constructor declaration but differed only in 
> which definitions they linked to? Even if dsymutil were to be able to merge 
> the declarations into a single one, LLDB would still have to support the case 
> where two constructor DIEs for the same type point to different definitions 
> depending on how they were used in the corresponding object files.
>
> One could instead have some new attribute on the various constructor 
> definitions specifying which constructor type it is, and then implement 
> @pavel's suggestion of attaching multiple `asm` labels to the 
> `CXXConstructorDecl`. To support that in LLDB we'd have to do a lookup in the 
> DWARF index and filter appropriately.

What about doing both? Have the declaration DIE specify the linkage names of 
both structor types (regardless of whether they are used in a particular CU -- 
just like with "normal" functions), and then have the definition DIEs specify 
which flavour of the structor are they defining?

> But it's unclear to me whether this is much better than the proposed patch. I 
> pinged the libcxx people regarding their policy of using abi-tags on 
> structures/namespaces

Yea.. I don't know. I don't really like it because it's still very much a 
secret clang-lldb handshake that will not work with other compilers or 
debuggers. But maybe it's better because it's not a three-way libc++-clang-lldb 
handshake and has a chance of working with libraries using abi tags in a 
different way than libc++?

I'm just the peanut gallery here, so feel free to overrule me.

> Some more findings on the Clang side: a very naive way to support multiple 
> `asm` labels is to adjust the `ManglingContext::mangleName` to be aware of 
> multiple `asm` labels and pick the correct one based on the `GlobalDecl` 
> we're mangling. But we'd need to somehow match `CtorType` to the 
> `AsmLabelAttr` in question. Which is possibly doable by encoding the 
> `CtorType` into the label. E.g., `asm("C2")` (?) Though that 
> seems hacky

A bit, though if the code change itself is simple/localized, then we might be 
able to get away with a new attribute, or an extension of the existing one 
(`A() __attribute((full_structor_asm("fooC1"), base_structor_asm("fooC2"))` or 
something -- it's not like people will be actually typing this code anyway...)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144181

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

[PATCH] D144181: [clang][DebugInfo] Add abi-tags on constructors/destructors as LLVM annotations

[Pavel Labath via Phabricator via cfe-commits]

labath added a comment.

In D144181#4139446 , @Michael137 
wrote:

>> Hmm, I'd sort of still be inclined to look further at the linkage name 
>> option - but maybe the existence of a situation where the usage is built 
>> with debug info, but the out of line ctor definitions are all in another 
>> library without debug info is a sufficiently motivating use case to justify 
>> the addition of these attributes to the ctor declaration. *shrug* Carry on 
>> then.
>
> That would've been convenient but I don't think we can get it right with 
> attaching the linkage name (even if we are willing to do the search for the 
> correct linkage name). That would essentially coalesce the possibly multiple 
> definitions of the constructor into a single one (since there's only ever a 
> single `CXXConstructorDecl` in the AST for a given constructor in the source)
>
> (CC @labath)

The thing that bothers me about this approach is that finding the right 
constructor in the object file requires reconstructing _exactly_ the mangled 
name of the constructor variant, and then looking up that name in the symbol 
table. The construction of the mangled name does not require getting just the 
abi tags of the constructor itself right. Rather, it requires knowing the abi 
tag annotations of all arguments (template and regular) of the constructor, and 
template arguments of all enclosing classes. Is the information in this patch 
enough to reconstruct this  name 
(`_ZN2S1I1AB4asdfE2S2I1BB4asdfEC2I1CB4asdfEET_1DB4asdf`, a.k.a, 
`S1::S2::S2(C[abi:asdf], D[abi:asdf])`)?

Even if it is, it still feels like we're not using dwarf the way it's meant to 
be used. Normal function would have DW_AT_linkage_name and DW_AT_addr 
attributes, and any one of those would be sufficient to find the function 
(unambiguously). The fact that we can't do that for constructors makes me 
believe that the problem is elsewhere. In a way, it feels like the right way to 
represent this situation in DWARF is to have two DW_TAG_subprogram DIEs -- 
given that there actually are two functions in the object file.

Of course that has a lot of problems of its own -- from breaking all existing 
debug info consumers to figuring out how to feed this information back into 
clang (in the LLDB expression parser). However, since we're contemplating 
extensions anyway, we could at least solve the first problem fairly easily by 
making the extra constructor an extension -- either an extra extension DIE, or 
just an extra attribute (with the other linkage name) on the existing 
constructor. For the second part, I could imagine some kind of an extension for 
the `asm` annotation for constructors, which would allow one to pass two 
mangled names instead of one. I have no idea how hard would it be to implement 
it though...

Anyway, that's just a dump of my thoughts on this topic. Feel free to disagree 
with this rant...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144181

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

[PATCH] D144181: [clang][DebugInfo] Add abi-tags on constructors/destructors as LLVM annotations

[Pavel Labath via Phabricator via cfe-commits]

labath added a comment.

In D144181#4140831 , @Michael137 
wrote:

>> The construction of the mangled name does not require getting just the abi 
>> tags of the constructor itself right. Rather, it requires knowing the abi 
>> tag annotations of all arguments (template and regular) of the constructor, 
>> and template arguments of all enclosing classes. Is the information in this 
>> patch enough to reconstruct this name 
>> (_ZN2S1I1AB4asdfE2S2I1BB4asdfEC2I1CB4asdfEET_1DB4asdf, a.k.a, 
>> S1::S2::S2(C[abi:asdf], D[abi:asdf]))?
>
> That's a fair point. I purposefully didn't support abi_tags on all possible 
> declarations because that would've ballooned the debug-info size too much. 
> And we get away with it because libc++ exclusively tags functions. But of 
> course there's no guarantee that they won't start tagging structures or 
> namespaces

Got it.

>> we could at least solve the first problem fairly easily by making the extra 
>> constructor an extension -- either an extra extension DIE, or just an extra 
>> attribute (with the other linkage name) on the existing constructor. For the 
>> second part, I could imagine some kind of an extension for the asm 
>> annotation for constructors, which would allow one to pass two mangled names 
>> instead of one. I have no idea how hard would it be to implement it though...
>
> I've thought about doing it that way for a bit too. It does seem like a more 
> consistent approach. What I concluded was that LLDB would have a hard time 
> with this sort of setup. E.g., how would we know which subprogram (and thus 
> which linkage name) to construct the single `CXXConstructorDecl` with? And 
> once we are set on a linkage name how would we make clang pick the other one 
> when appropriate?

Thanks for considering this.

This is what I meant by the "extension for the asm annotation for constructors" 
part. In principle, we could extend clang so that one can write something like 
this:

  class A {
A() asm("mangled_name_for_the_full_constructor", 
"mangled_name_for_the_base_constructor");
  };

and then clang would automatically pick the right mangled name depending on the 
context. (Exact syntax TBD, and one would need to figure out what to do with 
"allocating ctor" and "deleting dtor" variants). Once we had that, we generate 
that annotation using the data we parse from the DWARF. The exact method for 
that would depend on the chosen format, but it shouldn't be too complicated. I 
suspect the hardest part would be plumb this extra info all the way through 
clang and llvm.

In D144181#4140924 , @Michael137 
wrote:

> Although I don't know of a scenario where we ever explicitly need to call the 
> base object constructor from the expression evaluator.

It's definitely contrived, but I think one could write something like this:

  (lldb) expr --top-level class A : public virtual_class_from_DWARF {}
  (lldb) expr A().foo()


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144181

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

[PATCH] D117744: [Driver] Remove obsoleted -gz=zlib-gnu

[Pavel Labath via Phabricator via cfe-commits]

labath added a comment.

This (unsurprisingly) broke an lldb test for the gnu style compression. You 
didn't get the notification because the bot was already red at the time. I've 
already fixed the situation by converting the test to yaml, but I thought you 
may want to know what happened.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117744

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

[PATCH] D116113: Add LLDB synthetic child and summary scripts for llvm::SmallVector, llvm::Optional, llvm::ErrorOr and llvm::Expected.

[Pavel Labath via Phabricator via cfe-commits]

labath added a comment.

Seems fairly straight-forward (and definitely useful). Just a couple of quick 
comments.




Comment at: clang/utils/ClangDataFormat.py:28-35
+debugger.HandleCommand("type summary add -F 
ClangDataFormat.Optional_summary -x 'llvm::Optional<.*>'")
+debugger.HandleCommand("type summary add -F 
ClangDataFormat.SmallVector_summary -x 'llvm::SmallVector<.*>'")
+debugger.HandleCommand("type summary add -F 
ClangDataFormat.Expected_summary -x 'llvm::Expected<.*>'")
+debugger.HandleCommand("type summary add -F 
ClangDataFormat.ErrorOr_summary -x 'llvm::ErrorOr<.*>'")
+debugger.HandleCommand("type synthetic add -l ClangDataFormat.Optional -x 
'llvm::Optional<.*>'")
+debugger.HandleCommand("type synthetic add -l ClangDataFormat.SmallVector 
-x 'llvm::SmallVector<.*>'")
+debugger.HandleCommand("type synthetic add -l ClangDataFormat.Expected -x 
'llvm::Expected<.*>'")

I believe it's necessary to anchor these regexes to avoid them matching things 
like `std::vector>`



Comment at: clang/utils/ClangDataFormat.py:371-373
+self.error_type = target.FindFirstType('std::__1::error_code')
+if not self.error_type.IsValid():
+self.error_type = 
target.FindFirstType('std::error_code').GetPointerType()

maybe access as 
`valobj.GetChildMemberWithName('ErrorStorage').GetType().GetTemplateArgumentType(0)`
 ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116113

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

[PATCH] D116329: [clang-check] Adjust argument adjusters for clang-check to strip options blocking the static analyzer

[Pavel Labath via Phabricator via cfe-commits]

labath resigned from this revision.
labath added a comment.

I'm sorry, but I don't feel qualified to review this. Last time I saw this file 
was in 2013.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116329

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