Re: r342053 - [CodeGen] Align rtti and vtable data

2018-09-17 Thread Eric Christopher via cfe-commits 
Thanks for looking Jordan!

On Mon, Sep 17, 2018, 11:16 AM Jordan Rupprecht 
wrote:

> > Interesting, what kind of failures?
> >
> > If they are causing you problems, of course feel free to revert.
> >
> > Dave
>
> Turns out they are all real issues which running the test under asan
> mode flags as global-buffer-overflow. I'm guessing the over-alignment
> was hiding the bug as reads there would be zero initialized, but now
> they are reading other non-zero data and crashing on that.
>
> So, this patch just exposes some buggy code. Let's not revert it.
> Sorry for the false alarm!
>
> -- Jordan
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r342053 - [CodeGen] Align rtti and vtable data

2018-09-17 Thread Jordan Rupprecht via cfe-commits 
> Interesting, what kind of failures?
>
> If they are causing you problems, of course feel free to revert.
>
> Dave

Turns out they are all real issues which running the test under asan
mode flags as global-buffer-overflow. I'm guessing the over-alignment
was hiding the bug as reads there would be zero initialized, but now
they are reading other non-zero data and crashing on that.

So, this patch just exposes some buggy code. Let's not revert it.
Sorry for the false alarm!

-- Jordan


smime.p7s
Description: S/MIME Cryptographic Signature
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r342053 - [CodeGen] Align rtti and vtable data

2018-09-17 Thread David Green via cfe-commits 
Hello


Interesting, what kind of failures?


If they are causing you problems, of course feel free to revert.

Dave


From: Eric Christopher 
Sent: 17 September 2018 18:07:47
To: David Green
Cc: cfe-commits@lists.llvm.org
Subject: Re: r342053 - [CodeGen] Align rtti and vtable data

Hi David,

I'm seeing test failures after this patch. I'm trying to get a test case 
reduced, but can we revert until we figure it out?

Thanks!

-eric

On Wed, Sep 12, 2018 at 7:10 AM David Green via cfe-commits 
mailto:cfe-commits@lists.llvm.org>> wrote:
Author: dmgreen
Date: Wed Sep 12 07:09:06 2018
New Revision: 342053

URL: http://llvm.org/viewvc/llvm-project?rev=342053=rev
Log:
[CodeGen] Align rtti and vtable data

Previously the alignment on the newly created rtti/typeinfo data was largely
not set, meaning that DataLayout::getPreferredAlignment was free to overalign
it to 16 bytes. This causes unnecessary code bloat.

Differential Revision: https://reviews.llvm.org/D51416

Modified:
cfe/trunk/lib/CodeGen/CGVTT.cpp
cfe/trunk/lib/CodeGen/CGVTables.cpp
cfe/trunk/lib/CodeGen/CodeGenModule.cpp
cfe/trunk/lib/CodeGen/CodeGenModule.h
cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp
cfe/trunk/test/CodeGenCXX/microsoft-abi-vbtables.cpp
cfe/trunk/test/CodeGenCXX/vtable-align.cpp
cfe/trunk/test/CodeGenCXX/vtable-linkage.cpp

Modified: cfe/trunk/lib/CodeGen/CGVTT.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGVTT.cpp?rev=342053=342052=342053=diff
==
--- cfe/trunk/lib/CodeGen/CGVTT.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGVTT.cpp Wed Sep 12 07:09:06 2018
@@ -119,10 +119,10 @@ llvm::GlobalVariable *CodeGenVTables::Ge

   llvm::ArrayType *ArrayType =
 llvm::ArrayType::get(CGM.Int8PtrTy, Builder.getVTTComponents().size());
+  unsigned Align = CGM.getDataLayout().getABITypeAlignment(CGM.Int8PtrTy);

-  llvm::GlobalVariable *GV =
-CGM.CreateOrReplaceCXXRuntimeVariable(Name, ArrayType,
-  llvm::GlobalValue::ExternalLinkage);
+  llvm::GlobalVariable *GV = CGM.CreateOrReplaceCXXRuntimeVariable(
+  Name, ArrayType, llvm::GlobalValue::ExternalLinkage, Align);
   GV->setUnnamedAddr(llvm::GlobalValue::UnnamedAddr::Global);
   return GV;
 }

Modified: cfe/trunk/lib/CodeGen/CGVTables.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGVTables.cpp?rev=342053=342052=342053=diff
==
--- cfe/trunk/lib/CodeGen/CGVTables.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGVTables.cpp Wed Sep 12 07:09:06 2018
@@ -756,9 +756,11 @@ CodeGenVTables::GenerateConstructionVTab
   if (Linkage == llvm::GlobalVariable::AvailableExternallyLinkage)
 Linkage = llvm::GlobalVariable::InternalLinkage;

+  unsigned Align = CGM.getDataLayout().getABITypeAlignment(VTType);
+
   // Create the variable that will hold the construction vtable.
   llvm::GlobalVariable *VTable =
-CGM.CreateOrReplaceCXXRuntimeVariable(Name, VTType, Linkage);
+  CGM.CreateOrReplaceCXXRuntimeVariable(Name, VTType, Linkage, Align);
   CGM.setGVProperties(VTable, RD);

   // V-tables are always unnamed_addr.

Modified: cfe/trunk/lib/CodeGen/CodeGenModule.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenModule.cpp?rev=342053=342052=342053=diff
==
--- cfe/trunk/lib/CodeGen/CodeGenModule.cpp (original)
+++ cfe/trunk/lib/CodeGen/CodeGenModule.cpp Wed Sep 12 07:09:06 2018
@@ -3099,10 +3099,9 @@ CodeGenModule::GetAddrOfGlobal(GlobalDec
   IsForDefinition);
 }

-llvm::GlobalVariable *
-CodeGenModule::CreateOrReplaceCXXRuntimeVariable(StringRef Name,
-  llvm::Type *Ty,
-  llvm::GlobalValue::LinkageTypes Linkage) 
{
+llvm::GlobalVariable *CodeGenModule::CreateOrReplaceCXXRuntimeVariable(
+StringRef Name, llvm::Type *Ty, llvm::GlobalValue::LinkageTypes Linkage,
+unsigned Alignment) {
   llvm::GlobalVariable *GV = getModule().getNamedGlobal(Name);
   llvm::GlobalVariable *OldGV = nullptr;

@@ -3138,6 +3137,8 @@ CodeGenModule::CreateOrReplaceCXXRuntime
   !GV->hasAvailableExternallyLinkage())
 GV->setComdat(TheModule.getOrInsertComdat(GV->getName()));

+  GV->setAlignment(Alignment);
+
   return GV;
 }


Modified: cfe/trunk/lib/CodeGen/CodeGenModule.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenModule.h?rev=342053=342052=342053=diff
==
--- cfe/trunk/lib/CodeGen/CodeGenModule.h (original)
+++ cfe/trunk/lib/CodeGen/CodeGenModule.h Wed Sep 12 07:09:06 2018
@@ -764,7 +764,8 @@ public:
   /// bitcast to the new variable.
   llvm::GlobalVariable *
   CreateOrRepl

Re: r342053 - [CodeGen] Align rtti and vtable data

2018-09-17 Thread Eric Christopher via cfe-commits 
Hi David,

I'm seeing test failures after this patch. I'm trying to get a test case
reduced, but can we revert until we figure it out?

Thanks!

-eric

On Wed, Sep 12, 2018 at 7:10 AM David Green via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Author: dmgreen
> Date: Wed Sep 12 07:09:06 2018
> New Revision: 342053
>
> URL: http://llvm.org/viewvc/llvm-project?rev=342053=rev
> Log:
> [CodeGen] Align rtti and vtable data
>
> Previously the alignment on the newly created rtti/typeinfo data was
> largely
> not set, meaning that DataLayout::getPreferredAlignment was free to
> overalign
> it to 16 bytes. This causes unnecessary code bloat.
>
> Differential Revision: https://reviews.llvm.org/D51416
>
> Modified:
> cfe/trunk/lib/CodeGen/CGVTT.cpp
> cfe/trunk/lib/CodeGen/CGVTables.cpp
> cfe/trunk/lib/CodeGen/CodeGenModule.cpp
> cfe/trunk/lib/CodeGen/CodeGenModule.h
> cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
> cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp
> cfe/trunk/test/CodeGenCXX/microsoft-abi-vbtables.cpp
> cfe/trunk/test/CodeGenCXX/vtable-align.cpp
> cfe/trunk/test/CodeGenCXX/vtable-linkage.cpp
>
> Modified: cfe/trunk/lib/CodeGen/CGVTT.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGVTT.cpp?rev=342053=342052=342053=diff
>
> ==
> --- cfe/trunk/lib/CodeGen/CGVTT.cpp (original)
> +++ cfe/trunk/lib/CodeGen/CGVTT.cpp Wed Sep 12 07:09:06 2018
> @@ -119,10 +119,10 @@ llvm::GlobalVariable *CodeGenVTables::Ge
>
>llvm::ArrayType *ArrayType =
>  llvm::ArrayType::get(CGM.Int8PtrTy,
> Builder.getVTTComponents().size());
> +  unsigned Align = CGM.getDataLayout().getABITypeAlignment(CGM.Int8PtrTy);
>
> -  llvm::GlobalVariable *GV =
> -CGM.CreateOrReplaceCXXRuntimeVariable(Name, ArrayType,
> -
> llvm::GlobalValue::ExternalLinkage);
> +  llvm::GlobalVariable *GV = CGM.CreateOrReplaceCXXRuntimeVariable(
> +  Name, ArrayType, llvm::GlobalValue::ExternalLinkage, Align);
>GV->setUnnamedAddr(llvm::GlobalValue::UnnamedAddr::Global);
>return GV;
>  }
>
> Modified: cfe/trunk/lib/CodeGen/CGVTables.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGVTables.cpp?rev=342053=342052=342053=diff
>
> ==
> --- cfe/trunk/lib/CodeGen/CGVTables.cpp (original)
> +++ cfe/trunk/lib/CodeGen/CGVTables.cpp Wed Sep 12 07:09:06 2018
> @@ -756,9 +756,11 @@ CodeGenVTables::GenerateConstructionVTab
>if (Linkage == llvm::GlobalVariable::AvailableExternallyLinkage)
>  Linkage = llvm::GlobalVariable::InternalLinkage;
>
> +  unsigned Align = CGM.getDataLayout().getABITypeAlignment(VTType);
> +
>// Create the variable that will hold the construction vtable.
>llvm::GlobalVariable *VTable =
> -CGM.CreateOrReplaceCXXRuntimeVariable(Name, VTType, Linkage);
> +  CGM.CreateOrReplaceCXXRuntimeVariable(Name, VTType, Linkage, Align);
>CGM.setGVProperties(VTable, RD);
>
>// V-tables are always unnamed_addr.
>
> Modified: cfe/trunk/lib/CodeGen/CodeGenModule.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenModule.cpp?rev=342053=342052=342053=diff
>
> ==
> --- cfe/trunk/lib/CodeGen/CodeGenModule.cpp (original)
> +++ cfe/trunk/lib/CodeGen/CodeGenModule.cpp Wed Sep 12 07:09:06 2018
> @@ -3099,10 +3099,9 @@ CodeGenModule::GetAddrOfGlobal(GlobalDec
>IsForDefinition);
>  }
>
> -llvm::GlobalVariable *
> -CodeGenModule::CreateOrReplaceCXXRuntimeVariable(StringRef Name,
> -  llvm::Type *Ty,
> -  llvm::GlobalValue::LinkageTypes
> Linkage) {
> +llvm::GlobalVariable *CodeGenModule::CreateOrReplaceCXXRuntimeVariable(
> +StringRef Name, llvm::Type *Ty, llvm::GlobalValue::LinkageTypes
> Linkage,
> +unsigned Alignment) {
>llvm::GlobalVariable *GV = getModule().getNamedGlobal(Name);
>llvm::GlobalVariable *OldGV = nullptr;
>
> @@ -3138,6 +3137,8 @@ CodeGenModule::CreateOrReplaceCXXRuntime
>!GV->hasAvailableExternallyLinkage())
>  GV->setComdat(TheModule.getOrInsertComdat(GV->getName()));
>
> +  GV->setAlignment(Alignment);
> +
>return GV;
>  }
>
>
> Modified: cfe/trunk/lib/CodeGen/CodeGenModule.h
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenModule.h?rev=342053=342052=342053=diff
>
> ==
> --- cfe/trunk/lib/CodeGen/CodeGenModule.h (original)
> +++ cfe/trunk/lib/CodeGen/CodeGenModule.h Wed Sep 12 07:09:06 2018
> @@ -764,7 +764,8 @@ public:
>/// bitcast to the new variable.
>llvm::GlobalVariable *
>CreateOrReplaceCXXRuntimeVariable(StringRef Name, llvm::Type *Ty,
> -llvm::GlobalValue::LinkageTypes
> Linkage);
> +

r342053 - [CodeGen] Align rtti and vtable data

2018-09-12 Thread David Green via cfe-commits 
Author: dmgreen
Date: Wed Sep 12 07:09:06 2018
New Revision: 342053

URL: http://llvm.org/viewvc/llvm-project?rev=342053=rev
Log:
[CodeGen] Align rtti and vtable data

Previously the alignment on the newly created rtti/typeinfo data was largely
not set, meaning that DataLayout::getPreferredAlignment was free to overalign
it to 16 bytes. This causes unnecessary code bloat.

Differential Revision: https://reviews.llvm.org/D51416

Modified:
cfe/trunk/lib/CodeGen/CGVTT.cpp
cfe/trunk/lib/CodeGen/CGVTables.cpp
cfe/trunk/lib/CodeGen/CodeGenModule.cpp
cfe/trunk/lib/CodeGen/CodeGenModule.h
cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp
cfe/trunk/test/CodeGenCXX/microsoft-abi-vbtables.cpp
cfe/trunk/test/CodeGenCXX/vtable-align.cpp
cfe/trunk/test/CodeGenCXX/vtable-linkage.cpp

Modified: cfe/trunk/lib/CodeGen/CGVTT.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGVTT.cpp?rev=342053=342052=342053=diff
==
--- cfe/trunk/lib/CodeGen/CGVTT.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGVTT.cpp Wed Sep 12 07:09:06 2018
@@ -119,10 +119,10 @@ llvm::GlobalVariable *CodeGenVTables::Ge
 
   llvm::ArrayType *ArrayType =
 llvm::ArrayType::get(CGM.Int8PtrTy, Builder.getVTTComponents().size());
+  unsigned Align = CGM.getDataLayout().getABITypeAlignment(CGM.Int8PtrTy);
 
-  llvm::GlobalVariable *GV =
-CGM.CreateOrReplaceCXXRuntimeVariable(Name, ArrayType,
-  llvm::GlobalValue::ExternalLinkage);
+  llvm::GlobalVariable *GV = CGM.CreateOrReplaceCXXRuntimeVariable(
+  Name, ArrayType, llvm::GlobalValue::ExternalLinkage, Align);
   GV->setUnnamedAddr(llvm::GlobalValue::UnnamedAddr::Global);
   return GV;
 }

Modified: cfe/trunk/lib/CodeGen/CGVTables.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGVTables.cpp?rev=342053=342052=342053=diff
==
--- cfe/trunk/lib/CodeGen/CGVTables.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGVTables.cpp Wed Sep 12 07:09:06 2018
@@ -756,9 +756,11 @@ CodeGenVTables::GenerateConstructionVTab
   if (Linkage == llvm::GlobalVariable::AvailableExternallyLinkage)
 Linkage = llvm::GlobalVariable::InternalLinkage;
 
+  unsigned Align = CGM.getDataLayout().getABITypeAlignment(VTType);
+
   // Create the variable that will hold the construction vtable.
   llvm::GlobalVariable *VTable =
-CGM.CreateOrReplaceCXXRuntimeVariable(Name, VTType, Linkage);
+  CGM.CreateOrReplaceCXXRuntimeVariable(Name, VTType, Linkage, Align);
   CGM.setGVProperties(VTable, RD);
 
   // V-tables are always unnamed_addr.

Modified: cfe/trunk/lib/CodeGen/CodeGenModule.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenModule.cpp?rev=342053=342052=342053=diff
==
--- cfe/trunk/lib/CodeGen/CodeGenModule.cpp (original)
+++ cfe/trunk/lib/CodeGen/CodeGenModule.cpp Wed Sep 12 07:09:06 2018
@@ -3099,10 +3099,9 @@ CodeGenModule::GetAddrOfGlobal(GlobalDec
   IsForDefinition);
 }
 
-llvm::GlobalVariable *
-CodeGenModule::CreateOrReplaceCXXRuntimeVariable(StringRef Name,
-  llvm::Type *Ty,
-  llvm::GlobalValue::LinkageTypes Linkage) 
{
+llvm::GlobalVariable *CodeGenModule::CreateOrReplaceCXXRuntimeVariable(
+StringRef Name, llvm::Type *Ty, llvm::GlobalValue::LinkageTypes Linkage,
+unsigned Alignment) {
   llvm::GlobalVariable *GV = getModule().getNamedGlobal(Name);
   llvm::GlobalVariable *OldGV = nullptr;
 
@@ -3138,6 +3137,8 @@ CodeGenModule::CreateOrReplaceCXXRuntime
   !GV->hasAvailableExternallyLinkage())
 GV->setComdat(TheModule.getOrInsertComdat(GV->getName()));
 
+  GV->setAlignment(Alignment);
+
   return GV;
 }
 

Modified: cfe/trunk/lib/CodeGen/CodeGenModule.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenModule.h?rev=342053=342052=342053=diff
==
--- cfe/trunk/lib/CodeGen/CodeGenModule.h (original)
+++ cfe/trunk/lib/CodeGen/CodeGenModule.h Wed Sep 12 07:09:06 2018
@@ -764,7 +764,8 @@ public:
   /// bitcast to the new variable.
   llvm::GlobalVariable *
   CreateOrReplaceCXXRuntimeVariable(StringRef Name, llvm::Type *Ty,
-llvm::GlobalValue::LinkageTypes Linkage);
+llvm::GlobalValue::LinkageTypes Linkage,
+unsigned Alignment);
 
   llvm::Function *
   CreateGlobalInitOrDestructFunction(llvm::FunctionType *ty, const Twine ,

Modified: cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp?rev=342053=342052=342053=diff