Thank you for that! From: Shoaib Meenai [mailto:smee...@fb.com] Sent: Tuesday, January 8, 2019 4:48 PM To: David Majnemer <david.majne...@gmail.com> Cc: Keane, Erich <erich.ke...@intel.com>; cfe-commits@lists.llvm.org; Martin Storsjo <mar...@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.majne...@gmail.com<mailto:david.majne...@gmail.com>> Date: Tuesday, January 8, 2019 at 3:21 PM To: Shoaib Meenai <smee...@fb.com<mailto:smee...@fb.com>> Cc: "Keane, Erich" <erich.ke...@intel.com<mailto:erich.ke...@intel.com>>, "cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>" <cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>>, Martin Storsjo <mar...@martin.st<mailto:mar...@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 <smee...@fb.com<mailto:smee...@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.ke...@intel.com<mailto:erich.ke...@intel.com>> Date: Tuesday, January 8, 2019 at 1:04 PM To: Shoaib Meenai <smee...@fb.com<mailto:smee...@fb.com>>, "cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>" <cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>>, David Majnemer <david.majne...@gmail.com<mailto:david.majne...@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:smee...@fb.com<mailto:smee...@fb.com>] Sent: Tuesday, January 8, 2019 12:41 PM To: Keane, Erich <erich.ke...@intel.com<mailto:erich.ke...@intel.com>>; cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>; David Majnemer <david.majne...@gmail.com<mailto:david.majne...@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-boun...@lists.llvm.org<mailto:cfe-commits-boun...@lists.llvm.org>> on behalf of Shoaib Meenai via cfe-commits <cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>> Reply-To: Shoaib Meenai <smee...@fb.com<mailto:smee...@fb.com>> Date: Tuesday, January 8, 2019 at 12:39 PM To: Erich Keane <erich.ke...@intel.com<mailto:erich.ke...@intel.com>>, "cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>" <cfe-commits@lists.llvm.org<mailto:cfe-commits@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-boun...@lists.llvm.org<mailto:cfe-commits-boun...@lists.llvm.org>> on behalf of Erich Keane via cfe-commits <cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>> Reply-To: Erich Keane <erich.ke...@intel.com<mailto:erich.ke...@intel.com>> Date: Tuesday, January 8, 2019 at 10:48 AM To: "cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>" <cfe-commits@lists.llvm.org<mailto:cfe-commits@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@lists.llvm.org<mailto:cfe-commits@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=
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
