<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Sep 9, 2013 at 4:32 PM, Volker Simonis <span dir="ltr"><<a href="mailto:volker.simonis@gmail.com" target="_blank">volker.simonis@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>Hi Magnus,<br><br></div>thanks again for the review. Please see my comments inline:<br>
<div><div><div class="gmail_extra"><br><br><div class="gmail_quote"><div class="im">On Mon, Sep 9, 2013 at 12:23 PM, Magnus Ihse Bursie <span dir="ltr"><<a href="mailto:magnus.ihse.bursie@oracle.com" target="_blank">magnus.ihse.bursie@oracle.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div text="#000000" bgcolor="#FFFFFF">
<div>Hi Volker,<br>
<br>
Some more comments inlined.<br>
<br>
</div><div>
<blockquote type="cite">
<div dir="ltr">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). <br>
</div>
</blockquote>
<br></div>
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.<br>
<br>
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.<br>
<br>
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.</div></blockquote><div> <br></div></div><div>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".<br>
<br></div><div class="im"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div text="#000000" bgcolor="#FFFFFF"><div>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div>
<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
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.<br>
<br>
</blockquote>
<div><br>
</div>
<div>Yes, exactly. It is used in
'jdk/makefiles/GensrcX11Wrappers.gmk'
(MEMORY_MODEL_FLAG="$(COMP_MODE_OPTION)$*").<br>
</div>
</div>
</div>
</div>
</blockquote></div>
Once again the X11 wrappers. We should really make an effort and get
rid of them. :-/<div><br>
<br></div></div></blockquote></div><div>I don't mind:) <br></div><div class="im"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div text="#000000" bgcolor="#FFFFFF">
<div>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div>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.</div>
</div>
</div>
</div>
</blockquote></div>
Sounds good! Nice aligning with the existing macro.<br>
<br>
However, you forgot to change the name in <a href="http://spec.gmk.in" target="_blank">spec.gmk.in</a>. (At least in
the webrev you published.)</div></blockquote><div> </div></div><div>Good catch! I didn't realize it because the build in the stage repositories currently doesn’t reach the X11 wrappers.<br></div><div>I've changed it and moved it near the definition of 'COMPILER_SUPPORTS_TARGET_BITS_FLAG'</div>
<div class="im">
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div text="#000000" bgcolor="#FFFFFF"><div>
<br>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div>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.<br>
</div>
<div><br>
</div>
<div>Also, the workaround for autoconf bug <a href="http://lists.gnu.org/archive/html/autoconf/2010-07/msg00004.html" target="_blank">http://lists.gnu.org/archive/html/autoconf/2010-07/msg00004.html</a>
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.<br>
</div>
</div>
</div>
</div>
</blockquote>
<br></div>
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.<br>
<br>
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.<span><font color="#888888"><br>
<br></font></span></div></blockquote><div><br></div></div><div>Exactly.<br><br></div><div>Is it OK if I push it now to <a href="http://hg.openjdk.java.net/ppc-aix-port/ppc-aix-port/stage" target="_blank">http://hg.openjdk.java.net/ppc-aix-port/ppc-aix-port/stage</a> or is there anything you want to test first?</div>
</div></div></div></div></div></blockquote><div><br></div><div>Vladimir will push the changes because he also needs to re-generate the closed config files (thanks Vladimir).<br><br></div><div>Here's the final webrev:<br>
<br><a href="http://cr.openjdk.java.net/~simonis/webrevs/8024265.v3">http://cr.openjdk.java.net/~simonis/webrevs/8024265.v3</a><br><br></div><div>@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.<br>
<br></div><div>Regards,<br></div><div>Volker<br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><div><div class="gmail_extra">
<div class="gmail_quote">
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div text="#000000" bgcolor="#FFFFFF"><span><font color="#888888">
/Magnus<br>
</font></span></div>
</blockquote></div><br></div></div></div></div>
</blockquote></div><br></div></div>