RFR (M): 8024265: Enable new build on AIX (top level part)

Tue Sep 10 13:05:45 UTC 2013

Sorry, I missed your question. I can't speak for the aix-port repo, but I'm satisfied from the build point of view. 

/Magnus

10 sep 2013 kl. 14:22 skrev Volker Simonis <volker.simonis at gmail.com>:

> 
> 
> 
> On Mon, Sep 9, 2013 at 4:32 PM, Volker Simonis <volker.simonis at gmail.com> wrote:
>> Hi Magnus,
>> 
>> thanks again for the review. Please see my comments inline:
>> 
>> 
>> On Mon, Sep 9, 2013 at 12:23 PM, Magnus Ihse Bursie <magnus.ihse.bursie at oracle.com> wrote:
>>> Hi Volker,
>>> 
>>> Some more comments inlined.
>>> 
>>>> OK, common/autoconf/build-aux/autoconf-config.guess was too old and didn't knew about AIX 7 so it returned the default AIX fallback which is 'rs6000-ibm-aix'. I have now fixed 'autoconf-config.guess' to know about AIX 7 (a one character change which is already in autoconf-2.69 so we won't have         problems if you should ever update autoconf-config.guess).
>>> 
>>> Hmm... While more elegant, the idea was that autoconf-config.guess should be a strict copy of the config.guess file from the autoconf package. The whole idea of wrapping it in a custom config.guess was to avoid "forking" the autoconf config.guess, with small changes (like this) that would be hard to track and might get lost if we update to a newer version from the upstream autoconf file.
>>> 
>>> That being said, since we settled on autoconf 2.69, we really should update the file to config.guess from autoconf-2.69. I've started a test run on our internal test system with the config.guess from 2.69 to see if it breaks any existing platforms. If not, I suggest we do a fast integration of the new config.guess.
>>> 
>>> Can you hold on with your changes until a new config.guess comes in? Even if you're just making a small change that will be reverted soon after, I think it would set a unfortunate precedent.
>>  
>> That's OK for me. Actually my change still works without the changes in "config.guess" on AIX 5.3. And it will automatically start working once we get the new "config.guess".
>> 
>>>> 
>>>>> Ahrgh, all these proud compilers with their own ways of expressing the same functionality. :( I assume that you are using the COMP_MODE_OPTION in the jdk projct? I couldn't find any references to it in the Hotspot build changes, and otherwise there seems to be no reason to export it in the spec.gmk file.
>>>> 
>>>> Yes, exactly. It is used in 'jdk/makefiles/GensrcX11Wrappers.gmk' (MEMORY_MODEL_FLAG="$(COMP_MODE_OPTION)$*").
>>> Once again the X11 wrappers. We should really make an effort and get rid of them. :-/
>> 
>> I don't mind:) 
>>>> I fully agree with the criticism on the name:) After we already have 'COMPILER_SUPPORTS_TARGET_BITS_FLAG' I've simply renamed it to 'COMPILER_TARGET_BITS_FLAG'. I think that's much more appropriate and if you don't like it we should ask the one who invented 'COMPILER_SUPPORTS_TARGET_BITS_FLAG' :) And I had to set 'COMPILER_TARGET_BITS_FLAG' a little earlier  such that is availabel in PLATFORM_SET_COMPILER_TARGET_BITS_FLAGS.
>>> Sounds good! Nice aligning with the existing macro.
>>> 
>>> However, you forgot to change the name in spec.gmk.in. (At least in the webrev you published.)
>>  
>> Good catch! I didn't realize it because the build in the stage repositories currently doesn’t reach the X11 wrappers.
>> I've changed it and moved it near the definition of  'COMPILER_SUPPORTS_TARGET_BITS_FLAG'
>> 
>>> 
>>>> 2. After you pointed out that setting '-q64' as extra flags on the configure command line is not the way it is supposed to work I recalled that we also have this problem on older Linux/PPC64 boxes (e.g. SLES 10) where the default compiler produces 32-bit objects by default. To fix this problem as well, I've inserted a call to PLATFORM_SET_COMPILER_TARGET_BITS_FLAGS followed by a second call to AC_CHECK_SIZEOF([int *], [1111]) in the case where we would otherwise have bailed out because the "TESTED_TARGET_CPU_BITS" differs from the actual "OPENJDK_TARGET_CPU_BITS". I think this change should not               affect any existing platforms because it will only be triggered where we woould have bailed out with an error anyway.
>>>> 
>>>> Also, the workaround for autoconf bug http://lists.gnu.org/archive/html/autoconf/2010-07/msg00004.html               in AC_CHECK_SIZEOF isn't needed any more now that we require at least autoconf-2.69 because the problem was fixed in 2.67. And if I looked at it more carefully I must say that I don't understand the workaround at all. In my opinion, the test "x$SIZEOF_INT_P" != "x$ac_cv_sizeof_int_p" will always fail, because the AC_CHECK_SIZEOF macro only writes a define for SIZEOF_INT_P into "confdefs.h" (as can be seen in generated-configure.h) but it never defines it in the shell. And defining SIZEOF_INT_P in the configure shell script wouldn't help if the define written by the AC_CHECK_SIZEOF macro was wrong (as described in the bug). So Ithink the best is to remove the workaround and use               "ac_cv_sizeof_int_p" in the places where we used AC_CHECK_SIZEOF before.
>>> 
>>> Good to get rid of the old workaround. I agree, it looks kind of weird. I think I might have been behind some of the weirdness; I think I interpreted the autoconf documentation as if it should assign a variable SIZEOF_INT_P in the configure script, and that the $ac_* variables were internal variables that should not be directly accessed. In the current documentation, at least, the $ac_cv_sizeof* macro is officially mentioned so it should be safe to use.
>>> 
>>> However, your second relies on some internal autoconf magic, by     unsetting variables and defines. We've tried to avoid that, but at times there were no choice. Since we're about to fail anyway, and the code is more in place for future, strange platforms, it's probably no harm.
>> 
>> Exactly.
>> 
>> Is it OK if I push it now to http://hg.openjdk.java.net/ppc-aix-port/ppc-aix-port/stage or is there anything you want to test first?
> 
> Vladimir will push the changes because he also needs to re-generate the closed config files (thanks Vladimir).
> 
> Here's the final webrev:
> 
> http://cr.openjdk.java.net/~simonis/webrevs/8024265.v3
> 
> @Vladimir: I used autoconf 2.69 to create the webrev, so if you recreate the config files the diff to 'common/autoconf/generated-configure.sh' should be only the time-stamp field.
> 
> Regards,
> Volker
> 
>>  
>>> /Magnus
> 