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

Volker Simonis volker.simonis at gmail.com
Fri Sep 6 09:57:53 PDT 2013


Hi Magnus,

thanks a lot for the fast review!

Please see the new webrev here:

http://cr.openjdk.java.net/~simonis/webrevs/8024265.v2/

and my comments inline:

On Thu, Sep 5, 2013 at 2:07 PM, Magnus Ihse Bursie <
magnus.ihse.bursie at oracle.com> wrote:

> Hi Volker,
>
> Most of the changes look good. Thank you for the general house-keeping of
> shared code!
>
> Some comments inlined:
>
>
>  common/autoconf/build-aux/**config.guess
>>
>> On AIX 'config.guess' may return 'powerpc' as architecture but 'powerpc'
>> is
>> implicitly handled as 32-bit architecture in 'platform.m4' so we always
>> set
>> it to 'rs6000' which we then map to 'ppc64' in 'platform.m4'.
>>
>
> We already have a case for powerpc64 in platform.m4 (which I think was put
> there as a placeholder for future development, i.e. what you are doing
> right now :-)). Is there any reason why config.guess should not return
> that, rather than rs6000? Apart from minimizing the changes in platform.m4,
> at least to me powerpc64 is more comprehensible than rs6000.
>
>
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).

Now 'autoconf-config.guess' returns "powerpc" as architecture
("powerpc-ibm-aix7.1.0.0" on AIX 7 and "powerpc-ibm-aix5.3.0.0" on AIX 5.3)
and I'll rewrite it in 'config.guess' to "powerpc64" if we're running on a
64-bit kernel such that the "rs600" change in 'platform.m4' becomes
obsolete.


>  Added new variable COMP_MODE_OPTION to hold the option name for the
>> 32/64-bit compiler parameter which is -m on the current OpenJDK platforms
>> but -m for xlc on AIX.
>>
>
> (I think you mean that it is -q for xlc)
>
> Right.

> 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)$*").


> First of all, I think you should have a look at the function
> PLATFORM_SET_COMPILER_TARGET_**BITS_FLAGS in platform.m4, and how it is
> called from PLATFORM_SETUP_OPENJDK_TARGET_**BITS. I believe the proper
> way forward is to do as we do on Solaris, and always set the -q flag. Your
> current solution requires, as you say, the -q64 to be passed as extra flags
> to the configure command line, and that's not really the way it is supposed
> to work.
>
> Unfortunately, I'm afraid that just fixing these two functions won't
> suffice, since there are likely a few -m sprinkled around the code. So
> we're back to having the -m/-q being a variable. However, I'm not very fond
> of the name. "Compare mode what?" was my first reaction. If it is supposed
> to be a shorthand for "compiler mode", I think it's too vague. Compilers
> can have many modes. I don't have a good replacement name, but since it is
> used to prefix OPENJDK_TARGET_CPU_BITS, something along these lines would
> problably make it more clear what it is about.
>
>
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.

I've also checked that the only remaining "-m" flags are in platform
dependant code (i.e.macosx-only) only or in the devkit part which is
gcc-only anyway as far as I understand.

I've also slightly reworked the logic as follows:

1. On AIX I now always call PLATFORM_SET_COMPILER_TARGET_BITS_FLAGS as you
suggested and as it is done on Solaris. This fixes the AIX problem.
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.


>  common/autoconf/toolchain.m4
>>
>>     - Detect xlc compiler version on AIX.
>>
> In case of failure, this does:
>   AC_MSG_ERROR([Failed....])
> without any context to the poor user about what failed. Please write a
> more descriptive error message, along the lines of "Failed to determine xlc
> compiler version" or so.
>

Done:
        AC_MSG_ERROR([Failed to detect the compiler version of $COMPILER
....])


>
>  common/makefiles/**JavaCompilation.gmk
>>
>>     - Replace xargs by wc for the task of "..trim the whitespace from the
>>
>>     contents file, to see if it is empty.." because xargs has problems on
>>     AIX and wc should work just as well if not even better on any other
>>     OpenJDK platform.
>>
>
> The change itself looks good, but you added a comment:
>
>  (xargs has problems on AIX and seems oversized for this task anyway:)
>
> which only refers to old, removed code, so it wouldn't make any sense if
> your patch is applied. Such a comment will confuse the reader rather than
> help, I think, and would be better removed.
>
>
Agreed and removed.


>
>  common/makefiles/**NativeCompilation.gmk
>>
>> Fixed usage of AR_FLAGS which does not exist as variable by replacing it
>> with the existing ARFLAGS.
>>
> Good catch! It seems we've never been using ARFLAGS at all, which is a bit
> worrying. I wonder what this does for the existing platforms, now that we
> (potentially) start sending ar a bunch of flags...
>
>
I suppose it worked until now because the flags you were supposed to send
were the default settings anyway. I think we built with these changes on
all Oracle platforms (maybe except MacOS) without problems.

/Magnus
>

Is it OK if I'll push it to ppc-aix-port/stage or do you want to test it
first or submit it yourself?

Thanks and best regards,
Volker
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/attachments/20130906/9f31a40f/attachment-0001.html 


More information about the ppc-aix-port-dev mailing list