r350643 - Limit COFF 'common' emission to <=32 alignment types.

Wed Jan 9 06:00:34 PST 2019

Thank you for that!

From: Shoaib Meenai [mailto:smeenai at fb.com]
Sent: Tuesday, January 8, 2019 4:48 PM
To: David Majnemer <david.majnemer at gmail.com>
Cc: Keane, Erich <erich.keane at intel.com>; cfe-commits at lists.llvm.org; Martin Storsjo <martin at martin.st>
Subject: Re: r350643 - Limit COFF 'common' emission to <=32 alignment types.

I sent out https://reviews.llvm.org/D56466 to clarify the comment accordingly.

From: David Majnemer <david.majnemer at gmail.com<mailto:david.majnemer at gmail.com>>
Date: Tuesday, January 8, 2019 at 3:21 PM
To: Shoaib Meenai <smeenai at fb.com<mailto:smeenai at fb.com>>
Cc: "Keane, Erich" <erich.keane at intel.com<mailto:erich.keane at intel.com>>, "cfe-commits at lists.llvm.org<mailto:cfe-commits at lists.llvm.org>" <cfe-commits at lists.llvm.org<mailto:cfe-commits at lists.llvm.org>>, Martin Storsjo <martin at martin.st<mailto:martin at martin.st>>
Subject: Re: r350643 - Limit COFF 'common' emission to <=32 alignment types.

Yes, the MinGW toolchain can handle this by specifying the alignment of a common symbol using the aligncomm directive. The MSVC toolchain has no such mechanism.

This is why the check uses isKnownWindowsMSVCEnvironment.

On Tue, Jan 8, 2019 at 1:09 PM Shoaib Meenai <smeenai at fb.com<mailto:smeenai at fb.com>> wrote:
It checks for both OS=Win32 and Environment=MSVC, so that wouldn't cover other COFF environments. wbs (Martin Storsjo) mentioned on IRC that MinGW adds an aligncomm directive to specify alignment for common symbols, so perhaps that's part of it?

From: "Keane, Erich" <erich.keane at intel.com<mailto:erich.keane at intel.com>>
Date: Tuesday, January 8, 2019 at 1:04 PM
To: Shoaib Meenai <smeenai at fb.com<mailto:smeenai at fb.com>>, "cfe-commits at lists.llvm.org<mailto:cfe-commits at lists.llvm.org>" <cfe-commits at lists.llvm.org<mailto:cfe-commits at lists.llvm.org>>, David Majnemer <david.majnemer at gmail.com<mailto:david.majnemer at gmail.com>>
Subject: RE: r350643 - Limit COFF 'common' emission to <=32 alignment types.

Yep, exactly.  I looked, and isKnownWindowsMSVCEnvironment checks for OS=Win32, which I believe would be different for other architectures.

From: Shoaib Meenai [mailto:smeenai at fb.com<mailto:smeenai at fb.com>]
Sent: Tuesday, January 8, 2019 12:41 PM
To: Keane, Erich <erich.keane at intel.com<mailto:erich.keane at intel.com>>; cfe-commits at lists.llvm.org<mailto:cfe-commits at lists.llvm.org>; David Majnemer <david.majnemer at gmail.com<mailto:david.majnemer at gmail.com>>
Subject: Re: r350643 - Limit COFF 'common' emission to <=32 alignment types.

Ah, looks like you were originally checking for COFF, and then David suggested checking for MSVC instead? I'm curious about why, although I'm sure the suggestion is legit :)

From: cfe-commits <cfe-commits-bounces at lists.llvm.org<mailto:cfe-commits-bounces at lists.llvm.org>> on behalf of Shoaib Meenai via cfe-commits <cfe-commits at lists.llvm.org<mailto:cfe-commits at lists.llvm.org>>
Reply-To: Shoaib Meenai <smeenai at fb.com<mailto:smeenai at fb.com>>
Date: Tuesday, January 8, 2019 at 12:39 PM
To: Erich Keane <erich.keane at intel.com<mailto:erich.keane at intel.com>>, "cfe-commits at lists.llvm.org<mailto:cfe-commits at lists.llvm.org>" <cfe-commits at lists.llvm.org<mailto:cfe-commits at lists.llvm.org>>
Subject: Re: r350643 - Limit COFF 'common' emission to <=32 alignment types.

Why does this check for isKnownWindowsMSVCEnvironment specifically? Wouldn't any COFF target (windows-cygnus, windows-gnu, windows-itanium, etc.) have the same limitation, since it's an object file format issue and not an ABI issue?

From: cfe-commits <cfe-commits-bounces at lists.llvm.org<mailto:cfe-commits-bounces at lists.llvm.org>> on behalf of Erich Keane via cfe-commits <cfe-commits at lists.llvm.org<mailto:cfe-commits at lists.llvm.org>>
Reply-To: Erich Keane <erich.keane at intel.com<mailto:erich.keane at intel.com>>
Date: Tuesday, January 8, 2019 at 10:48 AM
To: "cfe-commits at lists.llvm.org<mailto:cfe-commits at lists.llvm.org>" <cfe-commits at lists.llvm.org<mailto:cfe-commits at lists.llvm.org>>
Subject: r350643 - Limit COFF 'common' emission to <=32 alignment types.

Author: erichkeane
Date: Tue Jan  8 10:44:22 2019
New Revision: 350643

URL: https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject-3Frev-3D350643-26view-3Drev&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=RNVKy_b0_Wgp_PTFDpvQXETsZdWubmT5SGnGz3GigS0&s=Ph9GOtRaQERmqyeJeAJTFwV3sg3q8fE05FlJ3qwNx4I&e=
Log:
Limit COFF 'common' emission to <=32 alignment types.

As reported in PR33035, LLVM crashes if given a common object with an
alignment of greater than 32 bits. This is because the COFF file format
does not support these alignments, so emitting them is broken anyway.

This patch changes any global definitions greater than 32 bit alignment
to no longer be in 'common'.

https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.llvm.org_show-5Fbug.cgi-3Fid-3D33035&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=RNVKy_b0_Wgp_PTFDpvQXETsZdWubmT5SGnGz3GigS0&s=ac1NEHuvztd6jSTCsOUJajkklfeyqdzW-xqtddJ-hvM&e=

Differential Revision: https://urldefense.proofpoint.com/v2/url?u=https-3A__reviews.llvm.org_D56391&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=RNVKy_b0_Wgp_PTFDpvQXETsZdWubmT5SGnGz3GigS0&s=AucP9Sp-DYHSaOP-sPfpAOrww3xwdh8FjQkHrLZhhyo&e=

Change-Id: I48609289753b7f3b58c5e2bc1712756750fbd45a

Added:
    cfe/trunk/test/CodeGen/microsoft-no-common-align.c
Modified:
    cfe/trunk/lib/CodeGen/CodeGenModule.cpp

Modified: cfe/trunk/lib/CodeGen/CodeGenModule.cpp
URL: https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_cfe_trunk_lib_CodeGen_CodeGenModule.cpp-3Frev-3D350643-26r1-3D350642-26r2-3D350643-26view-3Ddiff&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=RNVKy_b0_Wgp_PTFDpvQXETsZdWubmT5SGnGz3GigS0&s=gmTnEmW03ruG8LbJluf5Z4yQcxM64QP7Ce1VTnVqvPo&e=
==============================================================================

--- cfe/trunk/lib/CodeGen/CodeGenModule.cpp (original)
+++ cfe/trunk/lib/CodeGen/CodeGenModule.cpp Tue Jan  8 10:44:22 2019
@@ -3761,6 +3761,11 @@ static bool isVarDeclStrongDefinition(co
       }
     }
   }
+  // COFF doesn't support alignments greater than 32, so these cannot be
+  // in common.
+  if (Context.getTargetInfo().getTriple().isKnownWindowsMSVCEnvironment() &&
+      Context.getTypeAlignIfKnown(D->getType()) > 32)
+    return true;
   return false;
}

Added: cfe/trunk/test/CodeGen/microsoft-no-common-align.c
URL: https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_cfe_trunk_test_CodeGen_microsoft-2Dno-2Dcommon-2Dalign.c-3Frev-3D350643-26view-3Dauto&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=RNVKy_b0_Wgp_PTFDpvQXETsZdWubmT5SGnGz3GigS0&s=hzPmmVFbvg4OTEVpnQ5pIfy295Ne0-xAsctZs00WZgY&e=
==============================================================================
--- cfe/trunk/test/CodeGen/microsoft-no-common-align.c (added)
+++ cfe/trunk/test/CodeGen/microsoft-no-common-align.c Tue Jan  8 10:44:22 2019
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -triple x86_64-windows-msvc -emit-llvm -o - %s | FileCheck %s
+typedef float TooLargeAlignment __attribute__((__vector_size__(64)));
+typedef float NormalAlignment __attribute__((__vector_size__(4)));
+
+TooLargeAlignment TooBig;
+// CHECK: @TooBig = dso_local global <16 x float>  zeroinitializer, align 64
+NormalAlignment JustRight;
+// CHECK: @JustRight = common dso_local global <1 x float>  zeroinitializer, align 4


_______________________________________________
cfe-commits mailing list
cfe-commits at lists.llvm.org<mailto:cfe-commits at lists.llvm.org>
https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.llvm.org_cgi-2Dbin_mailman_listinfo_cfe-2Dcommits&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=RNVKy_b0_Wgp_PTFDpvQXETsZdWubmT5SGnGz3GigS0&s=Myn7SZhcOe32EZiKZr4ByJAZOoFl5aIfmWV9555Vh9A&e=

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190109/b6b33e41/attachment-0001.html>
