Bug 40722 - [4.5/4.6 Regression] ia32intrin.h defines of _rotl, _rotr conflict with target stdlib.h decls
Summary: [4.5/4.6 Regression] ia32intrin.h defines of _rotl, _rotr conflict with targe...
Status: RESOLVED INVALID
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 4.5.0
: P2 normal
Target Milestone: 4.5.3
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-07-11 22:07 UTC by Danny Smith
Modified: 2011-02-03 12:29 UTC (History)
6 users (show)

See Also:
Host: i686-pc-mingw32
Target: i686-pc-mingw32
Build: i686-pc-mingw32
Known to work:
Known to fail:
Last reconfirmed:


Attachments
Broken stdlib.h (4.58 KB, text/plain)
2010-03-28 10:14 UTC, jon_y
Details
log from buildbot (901 bytes, text/plain)
2010-03-28 10:17 UTC, jon_y
Details
gcc46-pr40722.patch (716 bytes, patch)
2011-02-03 10:05 UTC, Jakub Jelinek
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Danny Smith 2009-07-11 22:07:18 UTC
These defines in ia32intrin.h 

#define _lrotl(a,b)		__rold((a), (b))
#define _lrotr(a,b)		__rord((a), (b))
...
#define _rotl(a,b)		__rold((a), (b))
#define _rotr(a,b)		__rord((a), (b))


conflict with mingw32 stdlib.h which
declares those names as functions

_CRTIMP unsigned int __cdecl __MINGW_NOTHROW _rotl(unsigned int, int) __MINGW_ATTRIB_CONST;
_CRTIMP unsigned int __cdecl __MINGW_NOTHROW _rotr(unsigned int, int) __MINGW_ATTRIB_CONST;
_CRTIMP unsigned long __cdecl __MINGW_NOTHROW _lrotl(unsigned long, int) __MINGW_ATTRIB_CONST;
_CRTIMP unsigned long __cdecl __MINGW_NOTHROW _lrotr(unsigned long, int) __MINGW_ATTRIB_CONST;


This is caught by  g++.dg/other/i386-[23456].C
Comment 1 Richard Biener 2009-07-12 08:24:07 UTC
These were added by HJ.  Either we need to fixinclude stdlib.h or not define
these based on a configure test (I guess the former is more robust if
ia32intrin.h defines these only if they are not already defined).
Comment 2 H.J. Lu 2009-07-12 18:19:48 UTC
(In reply to comment #1)
> These were added by HJ.  Either we need to fixinclude stdlib.h or not define
> these based on a configure test (I guess the former is more robust if
> ia32intrin.h defines these only if they are not already defined).
> 

fixinclude sounds a good idea, but I don't know how to do it. Or
I can change ia32inintrin.h by not defining them for mingw.
Comment 3 Kai Tietz 2009-08-30 17:34:35 UTC
(In reply to comment #2)
> (In reply to comment #1)
> > These were added by HJ.  Either we need to fixinclude stdlib.h or not define
> > these based on a configure test (I guess the former is more robust if
> > ia32intrin.h defines these only if they are not already defined).
> > 
> 
> fixinclude sounds a good idea, but I don't know how to do it. Or
> I can change ia32inintrin.h by not defining them for mingw.
> 

I would vote for guarding the lrotl, lrotr by checks, if they aren't defined already.

There is an other issue about ia32intrin.h and mingw targets, but I assume, that it is true for other i386 and x86_64 targets, too. There are the functions __crc32b, __crc32w, and __crt32d, which are in general just available for a target that has SSE4.1 enabled (-mcrc). But the inlines aren't guarded and so the builtins __builtin_ia32_crc32qi (etc) don't have prototypes and so g++ will throw errors.

The following sample shows the issue pretty well.
t.cc:
#include <x86intrin.h>
int main()
{
  return 0;
}
Comment 4 nightstrike 2010-03-19 16:16:52 UTC
Can we fix this before 4.5 is released?
Comment 5 H.J. Lu 2010-03-20 14:34:48 UTC
I saw

_CRTIMP unsigned int __cdecl __MINGW_NOTHROW _rotl(unsigned int, int) __MINGW_ATTRIB_CONST;
_CRTIMP unsigned int __cdecl __MINGW_NOTHROW _rotr(unsigned int, int) __MINGW_ATTRIB_CONST;
_CRTIMP unsigned long __cdecl __MINGW_NOTHROW _lrotl(unsigned long, int) __MINGW_ATTRIB_CONST;
_CRTIMP unsigned long __cdecl __MINGW_NOTHROW _lrotr(unsigned long, int) __MINGW_ATTRIB_CONST;

There is no easy way to check it. I think they should be fixed by
fixinclude.
Comment 6 H.J. Lu 2010-03-20 18:01:58 UTC
A patch is posted at

http://gcc.gnu.org/ml/gcc-patches/2010-03/msg00936.html
Comment 7 H.J. Lu 2010-03-20 18:09:29 UTC
An updated patch is at

http://gcc.gnu.org/ml/gcc-patches/2010-03/msg00937.html
Comment 8 Kai Tietz 2010-03-23 12:32:02 UTC
(In reply to comment #7)
> An updated patch is at
> 
> http://gcc.gnu.org/ml/gcc-patches/2010-03/msg00937.html
> 

Patch is fine. It is absolutely necessary to support gcc's intrinsic headers for mingw. The fixinclude approach is one way to solve it and I am fine by it.
For mingw-w64 we used the #pragma push/pop_macro feature to make sure we declare function without getting problems by this define in ia32intrin.h.

Kai
Comment 9 hjl@gcc.gnu.org 2010-03-23 13:24:50 UTC
Subject: Bug 40722

Author: hjl
Date: Tue Mar 23 13:24:37 2010
New Revision: 157665

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=157665
Log:
Fix stdlib.h for mingw.

2010-03-23  H.J. Lu  <hongjiu.lu@intel.com>

	PR target/40722
	* mkfixinc.sh: Fix stdlib.h for mingw.

Modified:
    trunk/fixincludes/ChangeLog
    trunk/fixincludes/mkfixinc.sh

Comment 10 jon_y 2010-03-28 10:14:54 UTC
Created attachment 20228 [details]
Broken stdlib.h

Hi,

This patched caused problems for mingw-w64 stdlib.h, see attachment.
Comment 11 jon_y 2010-03-28 10:17:18 UTC
Created attachment 20229 [details]
log from buildbot

Here is the error output from one of the buildbot slaves.
Comment 12 H.J. Lu 2010-03-28 14:44:43 UTC
(In reply to comment #10)
> Created an attachment (id=20228) [edit]
> Broken stdlib.h
> 
> Hi,
> 
> This patched caused problems for mingw-w64 stdlib.h, see attachment.
> 

Someone from mingw64 should adjust fixincludes/mkfixinc.sh for mingw64.
Comment 13 Richard Biener 2010-03-28 15:34:18 UTC
(In reply to comment #12)
> (In reply to comment #10)
> > Created an attachment (id=20228) [edit]
> > Broken stdlib.h
> > 
> > Hi,
> > 
> > This patched caused problems for mingw-w64 stdlib.h, see attachment.
> > 
> 
> Someone from mingw64 should adjust fixincludes/mkfixinc.sh for mingw64.

HJ, please consider reverting your patch.
Comment 14 hjl@gcc.gnu.org 2010-03-28 16:41:15 UTC
Subject: Bug 40722

Author: hjl
Date: Sun Mar 28 16:40:50 2010
New Revision: 157784

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=157784
Log:
2010-03-28  H.J. Lu  <hongjiu.lu@intel.com>

	PR target/40722
	* mkfixinc.sh: Revert the last change for mingw.

Modified:
    trunk/fixincludes/ChangeLog
    trunk/fixincludes/mkfixinc.sh

Comment 15 Richard Biener 2010-04-06 11:20:26 UTC
GCC 4.5.0 is being released.  Deferring to 4.5.1.
Comment 16 Richard Biener 2010-07-31 09:29:33 UTC
GCC 4.5.1 is being released, adjusting target milestone.
Comment 17 Richard Biener 2010-12-16 13:03:28 UTC
GCC 4.5.2 is being released, adjusting target milestone.
Comment 18 Jakub Jelinek 2011-02-03 09:07:12 UTC
(In reply to comment #8)
> (In reply to comment #7)
> > An updated patch is at
> > 
> > http://gcc.gnu.org/ml/gcc-patches/2010-03/msg00937.html
> > 
> 
> Patch is fine. It is absolutely necessary to support gcc's intrinsic headers
> for mingw. The fixinclude approach is one way to solve it and I am fine by it.
> For mingw-w64 we used the #pragma push/pop_macro feature to make sure we
> declare function without getting problems by this define in ia32intrin.h.
> 
> Kai

Why can't you just
_CRTIMP unsigned int __cdecl __MINGW_NOTHROW (_rotl)(unsigned int, int) __MINGW_ATTRIB_CONST;
?
_rotl etc. from ia32intrin.h is a function-like macro, so the above prevents the expansion.
Is this still a problem on *mingw and *mingw64?  I mean, if x86intrin.h can't be included, it sounds like a serious problem for those that use this arch, and it is open for more than a year.
I guess H.J.'s patch is just too dangerous, it assumes that _rotl is the first declaration and _lrotr is the last one, I guess much better would be to ifdef out
each declaration separately, then there wouldn't be issues with unmatched #if/#endif.
Comment 19 Jakub Jelinek 2011-02-03 10:05:58 UTC
Created attachment 23233 [details]
gcc46-pr40722.patch

Modified H.J.'s patch which just adds ()s around _rotl/_rotr/_lrotl/_lrotr names in their prototypes, all I've tested is the sed command on mingw.org and mingw64 stdlib.h and eyeballed the differences.
Can anyone please test this both with mingw64 and mingw.org stdlib.h?
Thanks.
Comment 20 Kai Tietz 2011-02-03 12:29:18 UTC
So, as more as I think about it, come to the point that this fix-include for stdlib.h isn't something good. The correct solution here is that mingw.org fixes their stdlib.h header by () (as suggested by this fix-include sed command, which by the way works as expected without clobbering ifdef/else blocks).
The issue I see here is, that the local include directory (LOCAL_INCLUDE_DIR) comes before fix-include in include order. So for a native configuration, this won't solve anything as it won't be used at all.

So IMHO, this problem should be closed as invalid.