Request for review 7191817: -XX:+UseSerialGC -XX:+UseLargePages crashes with SIGFPE on Mac OS X

[Coleen Phillimore]

So I'm confused about one thing below.

On 10/25/2012 10:15 AM, Daniel D. Daugherty wrote:
> On 10/25/12 1:14 AM, Staffan Larsen wrote:
>> On 25 okt 2012, at 05:39, David Holmes<David.Holmes at oracle.com>  wrote:
>>
>>> Hi Harold,
>>>
>>> My only query is whether _ALLBSD_SOURCE is the right conditional for 
>>> this? Exactly which platform(s) don't support this?
>>>
>>> The reason I ask is that even the BSD sources (eg 
>>> ./os/bsd/vm/os_bsd.hpp) contain ifdefs for _ALL_BSD_SOURCE which 
>>> indicates that some bsd builds do not in fact set _ALLBSD_SOURCE ! 
>>> Which I find extremely confusing.
>> I actually think this is just leftovers from the port and should be 
>> removed. I think when the port to bsd was made, we started with the 
>> linux code and then added these ifdefs to show what the "orginal" 
>> code looked like while working on the new code. I may be wrong here 
>> and someone maybe has a better explanation. The code that is ifdef:ed 
>> away looks very similar to the linux code.
>>
>> I have a patch somewhere to remove this code, but I haven't gotten 
>> around to finish it up.
>>
>> /Staffan
>
> Staffan is correct. _ALL_BSD_SOURCE was indeed used to distinguish
> between the original Linux code and the changes specific to BSD.
> I left them in because it made it easier to re-audit the changes
> during the many resyncs with the bsd-port and macosx-port projects.
>
> If we ever get around to putting all the common UNIXish code in one
> place, then this kind of stuff can go away... Hey maybe the logical
> platform can be "COMMIX/commix" :-)
>
If we remove the code under _ALL_BSD_SOURCE, then the os_bsd.cpp file 
will be very different from the os_linux.cpp sources (which they are 
already with the ALL_BSD macros).  So in effect, these sources would not 
be good candidates to put in a common "unix" directory.   Or whatever we 
finally settle on calling this common unix directory.

Since the conditional compilation that Harold added to arguments.cpp 
matches what's in os_bsd.cpp, I think this should be left like this.   
If this conditional is removed in the future, the matching one in 
arguments.cpp can be adjusted at that time.   Having them not match 
before the cleanup seems dangerous to me.

Coleen
> Dan
>
>
>>> Thanks,
>>> David
>>>
>>> On 25/10/2012 12:22 AM, harold seigel wrote:
>>>> Please review this updated webrev:
>>>> http://cr.openjdk.java.net/~coleenp/bug_7191817_2/
>>>> <http://cr.openjdk.java.net/%7Ecoleenp/bug_7191817_2/>
>>>>
>>>> It incorporates Dave's suggestion to use the UNSUPPORTED_OPTION macro.
>>>>
>>>> Thanks, Harold
>>>>
>>>> On 10/24/2012 5:46 AM, David Holmes wrote:
>>>>> Hi Harold,
>>>>>
>>>>> We already have a macro for this:
>>>>>
>>>>> // Disable options not supported in this release, with a warning 
>>>>> if they
>>>>> // were explicitly requested on the command-line
>>>>> #define UNSUPPORTED_OPTION(opt, description) \
>>>>> do { \
>>>>> if (opt) { \
>>>>> if (FLAG_IS_CMDLINE(opt)) { \
>>>>> warning(description " is disabled in this release."); \
>>>>> } \
>>>>> FLAG_SET_DEFAULT(opt, false); \
>>>>> } \
>>>>> } while(0)
>>>>>
>>>>> We use this on platforms were not all features are supported.
>>>>>
>>>>> David
>>>>>
>>>>> On 24/10/2012 5:04 AM, harold seigel wrote:
>>>>>> Summary: Support for the -XX:+UseLargePages flag causes crashes 
>>>>>> on BSD
>>>>>> platforms such as MacOS X. This proposed change adds code to 
>>>>>> print out a
>>>>>> message if -XX:+UseLargePages is specified on this platform and 
>>>>>> to then
>>>>>> ignore the flag.
>>>>>>
>>>>>> Open webrev at http://cr.openjdk.java.net/~coleenp/bug_7191817
>>>>>> <http://cr.openjdk.java.net/%7Ecoleenp/bug_7191817>
>>>>>>
>>>>>> Bug link at http://bugs.sun.com/view_bug.do?bug_id=7191817
>>>>>>
>>>>>> This change was tested with JPRT and JCK tests. Also, tests were 
>>>>>> run by
>>>>>> hand on a MacOS X machine specifying the -XX:+UseLargePages flag 
>>>>>> to make
>>>>>> sure that the message was output and no crashes occurred.
>>>>>>
>>>>>> Thanks, Harold