Re: [urgent][jdk10] RFR: JDK-8198658 Docs still point to JDK 9 docs

[Martin Buchholz]

On Thu, Mar 1, 2018 at 11:50 AM, Erik Joelsson 
wrote:

>
> On 2018-02-26 12:57, joe darcy wrote:
>
>> Hi Magnus,
>>
>> Looks okay for now, but longer term should the version be queried from
>> the environment some way?
>>
>> The problem as I understand it is that the URL is dead until the docs
> team creates it, which doesn't necessarily happen in sync with us bumping
> the version number in the jdk/jdk repository. Perhaps that's ok early in
> the release?


If you take "always releasable" seriously, then there's no such thing as
"ok early in the release".  Why can't we always have up to date docs, and
the directory the docs live in is derived from the jdk sources (or
generated api docs) themselves? Seems like a small amount of release
engineering.

Re: RFR: JDK-8198243: Add build time check for global operator new/delete in object files

[Kim Barrett]

> On Mar 2, 2018, at 4:18 PM, Erik Joelsson  wrote:
> 
> Hello,
> 
> Here is a new version of this patch, reworked in several ways. It now 
> supports gcc, clang and solstudio. It uses nm instead of objdump, which is 
> more readily available in all our current build environments. The check now 
> uses mangled symbol names for all toolchain types which makes things 
> considerably faster. I also added a check for only finding symbols classified 
> as undefined. Otherwise we get false positives in operator_new.o in debug 
> builds.
> 
> I left the objdump addition to the devkit in there because it's a good thing 
> to have anyway for compare.sh.
> 
> Webrev: http://cr.openjdk.java.net/~erikj/8198243/webrev.02/
> 
> I have run this patch against current jdk/jdk and jdk/hs in Mach5. In jdk/jdk 
> the build fails as expected on Solaris, Linux and Mac and in jdk/hs, where 
> the fixes we are detecting are present, all builds succeeds.
> 
> /Erik

This looks good, with the same caveat as Coleen that I’m not a build expert.  
Thanks, Erik.

Re: RFR: JDK-8198243: Add build time check for global operator new/delete in object files

[Erik Joelsson]

Hello,

Here is a new version of this patch, reworked in several ways. It now 
supports gcc, clang and solstudio. It uses nm instead of objdump, which 
is more readily available in all our current build environments. The 
check now uses mangled symbol names for all toolchain types which makes 
things considerably faster. I also added a check for only finding 
symbols classified as undefined. Otherwise we get false positives in 
operator_new.o in debug builds.


I left the objdump addition to the devkit in there because it's a good 
thing to have anyway for compare.sh.


Webrev: http://cr.openjdk.java.net/~erikj/8198243/webrev.02/

I have run this patch against current jdk/jdk and jdk/hs in Mach5. In 
jdk/jdk the build fails as expected on Solaris, Linux and Mac and in 
jdk/hs, where the fixes we are detecting are present, all builds succeeds.


/Erik


On 2018-02-23 05:27, Magnus Ihse Bursie wrote:

On 2018-02-22 20:41, Erik Joelsson wrote:



On 2018-02-21 21:06, David Holmes wrote:

On 22/02/2018 4:07 AM, Erik Joelsson wrote:

Hello,


On 2018-02-20 21:33, David Holmes wrote:


a) how much time it adds to the build?

I have not done extensive testing, but on my Linux workstation with 
32 hw threads, building just hotspot release build from a clean 
workspace increased maybe 1 or 2 seconds (at around 90s total), but 
the variance was around the same amount as that.

b) why this doesn't work for Solaris Studio?

I didn't put a lot of effort into trying to figure it out. The 
check used was provided by Kim Barrett, for Linux only. I figured 
it would be simple enough to get it to work on mac and succeeded 
there. It should certainly be possible to implement a similar check 
on Solaris, but is it worth the time to do it? Both development 
time and increased build time on one of the slower build platforms?


Depends how concerned we are with detecting this problem in OS 
specific source code?


I investigated this some more. I was able to do it successfully, but 
the build time cost is way too large here. The culprit is c++filt on 
Solaris which is incredibly costly, and the gnu version does not 
demangle Solaris Studio symbols. I don't think we should do this on 
Solaris.

I agree, it's not worth it.

Not all programmer's mistakes are reasonable to catch in technical 
traps. It we *should* spend time on getting automatic tool for keeping 
code quality up (and, yes, I really do think we should), there's most 
likely to be much better areas to spend that effort in, in making a 
lot of prone-to-break scripts for catching a single kind of problem.


/Magnus



We could grep for the mangled strings for the operators instead, 
which is super fast. Problem is just figuring out all the possible 
combinations.


/Erik

To be honest I'm not sure this pulls its own weight regardless.

David


/Erik

Thanks,
David

On 21/02/2018 4:05 AM, Erik Joelsson wrote:

Hello,

This patch adds a build time check for uses of global operators 
new and delete in hotspot C++ code. The check is only run with 
toolchains GCC and Clang (Linux and Macos builds). I have also 
modified the Oracle devkit on Linux to add the necessary symlink 
so that objdump will get picked up by configure.


This change is depending on several fixes removing such uses that 
are currently in jdk/hs so this change will need to be pushed 
there as well.


Bug: https://bugs.openjdk.java.net/browse/JDK-8198243

Webrev: http://cr.openjdk.java.net/~erikj/8198243/webrev.01/

/Erik

Re: RFR 8198834: (ch) Enable java/nio/channels/spi/SelectorProvider/inheritedChannel/InheritedChannelTest.java on linux-x64

[Brian Burkhalter]

On Mar 1, 2018, at 11:52 PM, Alan Bateman  wrote:

> I assume the copyright range should be 2003, 2018 (to match Launcher.java) as 
> this is not a new file in 2018.

Will update before pushing.

> So I think you good to go and it's great to have the .so files removed from 
> the repos.

+1

Thanks,

Brian

Re: RFR: JDK-8198323 testing.md not updated for repository layout change

[Erik Joelsson]

Hello,

Nice fixes, some grammar notes:

building.md: 872: provide -> provides

testing.md: 57: JTReg test -> JTReg tests

68: tests roots -> test roots

77: "Here too can just `hotspot` be used as an alias for 
`hotspot/jtreg`" reads weird (and is missing punctuation), how about:

"`hotspot` can be used as an alias for `hotspot/jtreg` here as well."

/Erik

On 2018-03-02 04:48, Magnus Ihse Bursie wrote:
In make/doc/testing.md, the descriptions of the TEST option for jtreg 
tests has not been updated to account for the repository layout 
change.  For example, "hotspot/test:hotspot_gc" should be 
"test/hotspot/jtreg:hotspot_gc".


Bug: https://bugs.openjdk.java.net/browse/JDK-8198323
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8198323-update-testing-documentation/webrev.01


/Magnus

Re: RFR: JDK-8198862 Stop doing funky compilation stuff for dtrace

[Tim Bell]

Looks good to me as well.

Tim

On 03/02/18 07:29, Erik Joelsson wrote:

This looks good to me.

/Erik


On 2018-03-02 04:45, Magnus Ihse Bursie wrote:

On 2018-03-02 12:10, Magnus Ihse Bursie wrote:

On 2018-03-02 03:01, David Holmes wrote:

Hi Magnus,

On 1/03/2018 10:48 AM, Magnus Ihse Bursie wrote:

We're doing a lot of weird compilation stuff for dtrace. With this
patch, most of the weirdness is removed. The remaining calls to
$(CC) -E has been changed to $(CPP) to clarify that we do not
compile, we just use the precompiler.

One of the changes I made was to actually split up the last and
final dtrace call into a separate preprocessing step. However, this
uses the solaris studio preprocessor instead of the ancient system
preprocessor, which has changed behavior. A string like (&``_var)
is now expanded to (& ` ` _var), which is not accepted by dtrace.
:-( I have worked around this by adding the preprocessed output,
without the spaces, in two places. If anyone wants to dig deeper
into dtrace script file syntax, or C preprocessor magic, to avoid
this, let me know... (I'll just state that the "obvious" solution
of sending -Xs to the preprocessor to get old-style behavior does
not work: this just makes the solaris studio preprocessor call the
ancient preprocessor in turn, and we've gained nothing...)

Bug: https://bugs.openjdk.java.net/browse/JDK-8198862
WebRev:
http://cr.openjdk.java.net/~ihse/JDK-8198862-stop-doing-funky-dtrace-compilation-stuff/webrev.01






Why did you rename generateJvmOffsetsMain.c to
generateJvmOffsetsMain.cpp? It isn't a C++ program, it's just a C
program.

Yes, but so are generateJvmOffsets.cpp. :-& There was no point in
mixing a .cpp and .c file for this trivial build tool helper. In
fact, I don't even understand why they are two separate files -- if I
get the blessings from someone in hotspot, I'll gladly just
concatenate them into a single file.

Come to think about it, I don't care about the hotspot group's
blessing. ;-) I just moved the main function into the
generateJvmOffsets.cpp file. It was just silly having it as a separate
file.




! # Since we cannot generated JvmOffsets.cpp as part of the
gensrc step,

Comment doesn't read right.

Typo, should be "generate". I'll fix.


Updated.

I also restored the extra ( ) in ExecuteWithLog with redirection, and
added an additional ( ) for one case that was previously missing one.

Finally I also added the changes to dtrace that Erik requested for
JDK-8198859, but which was already pushed by that time.

New webrev:
http://cr.openjdk.java.net/~ihse/JDK-8198862-stop-doing-funky-dtrace-compilation-stuff/webrev.02


/Magnus

Re: RFR: JDK-8198862 Stop doing funky compilation stuff for dtrace

[Erik Joelsson]

This looks good to me.

/Erik


On 2018-03-02 04:45, Magnus Ihse Bursie wrote:

On 2018-03-02 12:10, Magnus Ihse Bursie wrote:

On 2018-03-02 03:01, David Holmes wrote:

Hi Magnus,

On 1/03/2018 10:48 AM, Magnus Ihse Bursie wrote:
We're doing a lot of weird compilation stuff for dtrace. With this 
patch, most of the weirdness is removed. The remaining calls to 
$(CC) -E has been changed to $(CPP) to clarify that we do not 
compile, we just use the precompiler.


One of the changes I made was to actually split up the last and 
final dtrace call into a separate preprocessing step. However, this 
uses the solaris studio preprocessor instead of the ancient system 
preprocessor, which has changed behavior. A string like (&``_var) 
is now expanded to (& ` ` _var), which is not accepted by dtrace. 
:-( I have worked around this by adding the preprocessed output, 
without the spaces, in two places. If anyone wants to dig deeper 
into dtrace script file syntax, or C preprocessor magic, to avoid 
this, let me know... (I'll just state that the "obvious" solution 
of sending -Xs to the preprocessor to get old-style behavior does 
not work: this just makes the solaris studio preprocessor call the 
ancient preprocessor in turn, and we've gained nothing...)


Bug: https://bugs.openjdk.java.net/browse/JDK-8198862
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8198862-stop-doing-funky-dtrace-compilation-stuff/webrev.01 





Why did you rename generateJvmOffsetsMain.c to 
generateJvmOffsetsMain.cpp? It isn't a C++ program, it's just a C 
program.
Yes, but so are generateJvmOffsets.cpp. :-& There was no point in 
mixing a .cpp and .c file for this trivial build tool helper. In 
fact, I don't even understand why they are two separate files -- if I 
get the blessings from someone in hotspot, I'll gladly just 
concatenate them into a single file.
Come to think about it, I don't care about the hotspot group's 
blessing. ;-) I just moved the main function into the 
generateJvmOffsets.cpp file. It was just silly having it as a separate 
file.




! # Since we cannot generated JvmOffsets.cpp as part of the 
gensrc step,


Comment doesn't read right.

Typo, should be "generate". I'll fix.


Updated.

I also restored the extra ( ) in ExecuteWithLog with redirection, and 
added an additional ( ) for one case that was previously missing one.


Finally I also added the changes to dtrace that Erik requested for 
JDK-8198859, but which was already pushed by that time.


New webrev:
http://cr.openjdk.java.net/~ihse/JDK-8198862-stop-doing-funky-dtrace-compilation-stuff/webrev.02 



/Magnus

Re: RFR: JDK-8198323 testing.md not updated for repository layout change

[Erik Helin]

On 03/02/2018 01:48 PM, Magnus Ihse Bursie wrote:
In make/doc/testing.md, the descriptions of the TEST option for jtreg 
tests has not been updated to account for the repository layout change. 
For example, "hotspot/test:hotspot_gc" should be 
"test/hotspot/jtreg:hotspot_gc".


Bug: https://bugs.openjdk.java.net/browse/JDK-8198323
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8198323-update-testing-documentation/webrev.01 


+example, `:tier1` will expand to `jtreg:$(TOPDIR)/test/jdk:tier1
+jtreg:$(TOPDIR)/test/langtools:tier1 jtreg:$(TOPDIR)/test/nashorn:tier1
+jtreg:$(TOPDIR)/test/jaxp:tier1`.

:tier1 will now actually include `jtreg:$(TOPDIR)/test/hotspot:tier1` as 
well since Christian's commit:

http://hg.openjdk.java.net/jdk/jdk/rev/75f4ad82866c

While you are at it, you might want to change all occurrences of 
VM_OTIONS to VM_OPTIONS :)


Thanks for updating the documentation!
Erik


/Magnus

Re: RFR: JDK-8198844 Clean up GensrcX11Wrappers

[Erik Joelsson]

Adding 2d-dev in the hopes of getting some input from component team. 
Seems like they should be aware of us removing the support for multiple 
data models.


Looks like you left a debug message at line 40 in GensrcX11Wrappers.gmk.

/Erik

On 2018-03-02 03:00, Magnus Ihse Bursie wrote:

On 2018-03-02 00:02, Erik Joelsson wrote:

Hello,

In xlibtypes.txt comment, should it be sizes-64.txt?


Yes, good catch.



Generating both 32 and 64 seems a bit outdated at this point. Surely 
this is a remnant of bundling 64 and 32 bit together on Solaris in 
the past? Perhaps someone in 2d can answer this? Would be nice to be 
able to clean up that part as well if possible.
Yes, you are right. We should clean up that as well. I was just too 
conservative. I've now actually checked what happens when you generate 
both 32 and 64 bit versions, and the result is that instead of code like:

    public static int getSize() { return 96; }
we get code like this:
    public static int getSize() { return ((XlibWrapper.dataModel == 
32)?(80):(96)); }


Since we do no longer support multiple data models for the same build, 
this is just unnecessary. In fact, that leads to an even better 
cleanup, since we will always need just a single input file.


I also wrapped the tool calls in ExecuteWithLog.

Updated webrev:
http://cr.openjdk.java.net/~ihse/JDK-8198844-clean-up-GensrcX11Wrappers/webrev.02 



/Magnus

Re: RFR: JDK-8198724 Refactor FLAGS handling in configure

[Erik Joelsson]

On 2018-03-02 01:54, Magnus Ihse Bursie wrote:


I actually don't know of any cases where CFLAGS ordering is relevant. 
(LIBS is another issue) It's good to be aware of such issues, so 
please enlighten me. :-)
If you have a flag that enables a feature and the corresponding 
disabling flag of the same, like -Wfoo -Wno-foo, then the latter 
overrides the former. Change the order and you get the opposite behavior.


-I directories are searched in order so if the same header name is found 
in multiple directories, you get differences.


/Erik

RFR: JDK-8198323 testing.md not updated for repository layout change

[Magnus Ihse Bursie]

In make/doc/testing.md, the descriptions of the TEST option for jtreg 
tests has not been updated to account for the repository layout change.  
For example, "hotspot/test:hotspot_gc" should be 
"test/hotspot/jtreg:hotspot_gc".


Bug: https://bugs.openjdk.java.net/browse/JDK-8198323
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8198323-update-testing-documentation/webrev.01


/Magnus

Re: RFR: JDK-8198862 Stop doing funky compilation stuff for dtrace

[Magnus Ihse Bursie]

On 2018-03-02 12:10, Magnus Ihse Bursie wrote:

On 2018-03-02 03:01, David Holmes wrote:

Hi Magnus,

On 1/03/2018 10:48 AM, Magnus Ihse Bursie wrote:
We're doing a lot of weird compilation stuff for dtrace. With this 
patch, most of the weirdness is removed. The remaining calls to 
$(CC) -E has been changed to $(CPP) to clarify that we do not 
compile, we just use the precompiler.


One of the changes I made was to actually split up the last and 
final dtrace call into a separate preprocessing step. However, this 
uses the solaris studio preprocessor instead of the ancient system 
preprocessor, which has changed behavior. A string like (&``_var) is 
now expanded to (& ` ` _var), which is not accepted by dtrace. :-( I 
have worked around this by adding the preprocessed output, without 
the spaces, in two places. If anyone wants to dig deeper into dtrace 
script file syntax, or C preprocessor magic, to avoid this, let me 
know... (I'll just state that the "obvious" solution of sending -Xs 
to the preprocessor to get old-style behavior does not work: this 
just makes the solaris studio preprocessor call the ancient 
preprocessor in turn, and we've gained nothing...)


Bug: https://bugs.openjdk.java.net/browse/JDK-8198862
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8198862-stop-doing-funky-dtrace-compilation-stuff/webrev.01 




Why did you rename generateJvmOffsetsMain.c to 
generateJvmOffsetsMain.cpp? It isn't a C++ program, it's just a C 
program.
Yes, but so are generateJvmOffsets.cpp. :-& There was no point in 
mixing a .cpp and .c file for this trivial build tool helper. In fact, 
I don't even understand why they are two separate files -- if I get 
the blessings from someone in hotspot, I'll gladly just concatenate 
them into a single file.
Come to think about it, I don't care about the hotspot group's blessing. 
;-) I just moved the main function into the generateJvmOffsets.cpp file. 
It was just silly having it as a separate file.




! # Since we cannot generated JvmOffsets.cpp as part of the 
gensrc step,


Comment doesn't read right.

Typo, should be "generate". I'll fix.


Updated.

I also restored the extra ( ) in ExecuteWithLog with redirection, and 
added an additional ( ) for one case that was previously missing one.


Finally I also added the changes to dtrace that Erik requested for 
JDK-8198859, but which was already pushed by that time.


New webrev:
http://cr.openjdk.java.net/~ihse/JDK-8198862-stop-doing-funky-dtrace-compilation-stuff/webrev.02

/Magnus

Re: RFR: JDK-8198859 Use elfedit to silence linker warnings on solaris

[Magnus Ihse Bursie]

On 2018-03-01 23:17, Erik Joelsson wrote:

Hello Magnus,

This is a nice fix!

I would prefer if the shell expression wasn't executed on every 
re-make. In this case it can easily be fixed by changing := to =. 
There is only one use of DTRACE_ELFEDIT_COMMANDS so it will only be 
evaluated once, and since it's in a recipe line, it will only get 
evaluated if that recipe is run. For better styling, perhaps it should 
be renamed with camel humps to make it more apparent that it's a macro.


Stylewise, I would also argue that the indentation on 149 should be 4 
spaces. One could say foreach qualifies as a logic indent, but I would 
say this is a broken up one liner. If foreach had started on a new 
line, then 2 space would have been ok, and in that case I would also 
have liked the closing brace on a new line.

Erik

I had already pushed the changeset after Tim's review. I will fold your 
suggested changes into the patch for JDK-8198862.


/Magnus



/Erik

On 2018-03-01 02:01, Magnus Ihse Bursie wrote:
Solaris builds have always produced a lot of warnings when linking, 
like this:


ld: fatal: symbol '__JvmOffsets' has differing types:
    (file 
/export/home/magnusi/hg/sandbox-cflags/build/solaris-sparcv9/hotspot/variant-server/libjvm/objs/JvmOffsets.o 
type=OBJT; file 
/export/home/magnusi/hg/sandbox-cflags/build/solaris-sparcv9/hotspot/variant-server/libjvm/objs/dtrace_jhelper.o 
type=FUNC);


This is due to an unresolved bug in dtrace. This bug has been 
reported on the dtrace team in 2014, but no solution have been coming 
forth. :-( However, I just discovered that we can actually use 
elfedit to fix the type of the fields that the linker is complaining 
about on $(DTRACE_JHELPER_OBJ).


That will make the linker quiet.

Bug: https://bugs.openjdk.java.net/browse/JDK-8198859
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8198859-dtrace-warnings-workaround/webrev.01


/Magnus

Re: RFR: JDK-8198862 Stop doing funky compilation stuff for dtrace

[Magnus Ihse Bursie]

On 2018-03-02 03:01, David Holmes wrote:

Hi Magnus,

On 1/03/2018 10:48 AM, Magnus Ihse Bursie wrote:
We're doing a lot of weird compilation stuff for dtrace. With this 
patch, most of the weirdness is removed. The remaining calls to $(CC) 
-E has been changed to $(CPP) to clarify that we do not compile, we 
just use the precompiler.


One of the changes I made was to actually split up the last and final 
dtrace call into a separate preprocessing step. However, this uses 
the solaris studio preprocessor instead of the ancient system 
preprocessor, which has changed behavior. A string like (&``_var) is 
now expanded to (& ` ` _var), which is not accepted by dtrace. :-( I 
have worked around this by adding the preprocessed output, without 
the spaces, in two places. If anyone wants to dig deeper into dtrace 
script file syntax, or C preprocessor magic, to avoid this, let me 
know... (I'll just state that the "obvious" solution of sending -Xs 
to the preprocessor to get old-style behavior does not work: this 
just makes the solaris studio preprocessor call the ancient 
preprocessor in turn, and we've gained nothing...)


Bug: https://bugs.openjdk.java.net/browse/JDK-8198862
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8198862-stop-doing-funky-dtrace-compilation-stuff/webrev.01 



Why did you rename generateJvmOffsetsMain.c to 
generateJvmOffsetsMain.cpp? It isn't a C++ program, it's just a C 
program.
Yes, but so are generateJvmOffsets.cpp. :-& There was no point in mixing 
a .cpp and .c file for this trivial build tool helper. In fact, I don't 
even understand why they are two separate files -- if I get the 
blessings from someone in hotspot, I'll gladly just concatenate them 
into a single file.
I agree the logic is quite confusing. I think this build logic was 
victim of the CPP_FLAGS (meaning C preprocessor) to CXX_FLAGS (meaning 
C++ flags) renaming. But this is a trivial C program and should 
require trivial C compiler flags. I don't see it should be being built 
with all the JVM_CFLAGS. The latter may be harmless but it seems wrong 
to lump this in together with other things.


Actually, no. Or, maybe it was a victim of CPP_FLAGS/CXX_FLAGS 
confusion, too. But the JVM_CFLAGS *are* needed. Otherwise I'd removed 
them, trust me. And I would have moved the entire piece of code to 
gensrc, where it belongs. (This is just about compiling a build tool 
that will generate source code that should be later compiled, typical 
gensrc stuff).


But, this file includes a lot of hotspot include files. And for that to 
work, we need the -I and -D flags from JVM_CFLAGS. We probably don't 
need any other parts of the JVM_CFLAGS, so in theory, we could probably 
split JVM_CFLAGS into a "defines and include paths" part, and a "rest" 
part. But I would not bet on it, suddenly you'd have some kind of option 
(-xc99?) that modifies the parsing of the include files...


This is the general problem with all dtrace stuff, it needs to poke it's 
fingers deep down in the libjvm. :(




make/hotspot/lib/CompileDtracePreJvm.gmk

! # Since we cannot generated JvmOffsets.cpp as part of the gensrc 
step,


Comment doesn't read right.

Typo, should be "generate". I'll fix.

/Magnus


Thanks,
David



/Magnus

Re: RFR: JDK-8198844 Clean up GensrcX11Wrappers

[Magnus Ihse Bursie]

On 2018-03-02 00:02, Erik Joelsson wrote:

Hello,

In xlibtypes.txt comment, should it be sizes-64.txt?


Yes, good catch.



Generating both 32 and 64 seems a bit outdated at this point. Surely 
this is a remnant of bundling 64 and 32 bit together on Solaris in the 
past? Perhaps someone in 2d can answer this? Would be nice to be able 
to clean up that part as well if possible.
Yes, you are right. We should clean up that as well. I was just too 
conservative. I've now actually checked what happens when you generate 
both 32 and 64 bit versions, and the result is that instead of code like:

    public static int getSize() { return 96; }
we get code like this:
    public static int getSize() { return ((XlibWrapper.dataModel == 
32)?(80):(96)); }


Since we do no longer support multiple data models for the same build, 
this is just unnecessary. In fact, that leads to an even better cleanup, 
since we will always need just a single input file.


I also wrapped the tool calls in ExecuteWithLog.

Updated webrev:
http://cr.openjdk.java.net/~ihse/JDK-8198844-clean-up-GensrcX11Wrappers/webrev.02

/Magnus

Re: RFR: JDK-8198724 Refactor FLAGS handling in configure

[Magnus Ihse Bursie]

On 2018-03-02 00:46, Erik Joelsson wrote:

Hello Magnus,

Nice to finally see this posted! Overall very nice improvement.

Thank you.


I would advocate a simple bash replace to remove the dots, like this:
MACOSX_VERSION_MIN_NODOTS=${MACOSX_VERSION_MIN//\./}


Ok, I updated this. I see no need to post another review for this.

* On macosx/clang, the JVM_CFLAGS for the build toolchain will now 
also get an -fPIC (this was mysteriously missing before).
* On windows/microsoft, the build toolchain will now get -nologo 
added as well, and also -homeparams for debug builds.
* On solaris/solstudio, the JDKEXE flags will now include -errfmt, 
and CFLAGS_JDKEXE will include -errtags=yes. The duplication of 
-errtags=yes in CXXFLAGS_JDKLIB is removed.
* On solaris/solstudio, we now always use -KPIC as pic flag. 
(Previously, we used -xcode=pic32 on sparc, but this is equivalent to 
-KPIC, so there's no reason for a special case.)
* On solaris/solstudio, we now use e.g. "-Wc,-Qrm-s" instead of 
"-Qoption cg -Qrm-s" for C++ as well as for C code. The two formats 
are equivalent (for passing options to a certain compilation phase), 
and there was no reason to use different ones for C and C++.


And for clarity, I have also renamed some exported flags to clearly 
show that they are consumed by LIBJSIG and ADLC. I have also renamed 
"/STACK" on the Microsoft linker to "-stack", to match how we write 
options elsewhere.


I believe none of these should have any real effect, but if anything, 
they could probably be considered bug fixes.


I have considered whitespace and ordering differences as irrelevant.

In some cases ordering may be relevant, hopefully the COMPARE_BUILD 
run will uncover if that's the case. If those are ok, then I'm 
basically happy with this transformation.
COMPARE_BUILD runs on our internal mach5 system showed no discrepancies. 
(This was the reason I fixed the COMPARE_BUILD system for no-change builds.)


I actually don't know of any cases where CFLAGS ordering is relevant. 
(LIBS is another issue) It's good to be aware of such issues, so please 
enlighten me. :-)


I noticed one thing, though, was that a trivial sort of the flags before 
writing them to spec.gmk didn't work. I did this (and similarly on a 
patched baseline) to facilitate my comparison, but it was not 
compilable. The reason was that a few flags (actually, currently only on 
clang/macosx) had arguments that was space separated from the options. 
For most flags, we do not have this scenario, which I think is good. It 
makes it easier to see what is a "single" flag.


I have manually verified that the generated spec.gmk and 
buildjdk-spec.gmk matches this (no changes apart from the one listed 
above) for linux/x64/gcc, solaris/sparcv9/solstudio, 
windows/x64/microsoft and macosx/x64/clang, for release and fastdebug.


I am also currently running a test job using the COMPARE_BUILD 
functionality on our internal build system.


I would appreciate if comments are more in the form of "think of this 
for subsequent updates to the flag handling" rather than requests to 
change this patch, at least for requests that are just not small 
fixes. (I've been juggling this for a while, trying to get it right...)


Bug: https://bugs.openjdk.java.net/browse/JDK-8198724
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8198724-refactor-flags-step-1/webrev.01



Comments on things I saw, not necessarily needing fixes now:

In flags.m4, MACHINE_FLAG and COMPILER_TARGET_BITS_FLAG are redundant 
without comment.
Oh, how I wish this was true. :-( COMPILER_TARGET_BITS_FLAG is actually 
exported for use in (you guess!) GensrcX11Wrappers.gmk. (Guess why I was 
targeting that for cleanup :-)).


When the GensrcX11Wrappers fix is in, I intend to clean up this.



flags-cflags.m4, FLAGS_SETUP_SHARED_LIBS, comments out of 
date/irrelevant like:

# Default works for linux, might work on other platforms as well.
I'll go through those at a later date. For the moment, 
FLAGS_SETUP_SHARED_LIBS was "good enough" to be left alone.



Solaris -errtags is not needed in CFLAGS_WARNINGS_ARE_ERRORS.
Yes. I'm currently working on a follow-up patch were I redesign the 
warnings handling, including -errtags.
Given that TOOLCHAIN_MINIMUM_VERSION_gcc="4.7", perhaps we can remove 
the check for -Wno-X on gcc 4.4.


Good find. It would be nice to get rid of it.

In fact, I wonder if it is possible to raise the minimum gcc version to 
4.8 for JDK11. That would help us get rid of even more gcc version 
checks for broken/bad behaviour in pre-4.8 gcc.


As you can see, there's a lot of follow-up cleaning left to do. This 
refactoring was scary enough that I wanted to minimize the changes done 
in this first step.


/Magnus



/Erik


/Magnus

Re: RFR JDK-8197533 move javax.transaction.xa into its own module

[Alan Bateman]

On 28/02/2018 19:54, Lance Andersen wrote:

:


Is there any XA text from the original JTA spec that should be added 
to the module description as part of this? Another way to ask this is 
whether the JTA 1.3 drops any text dealing with the XA part.
Still waiting to see what changes are made to the PDF spec, that is 
still needing to be completed. So I think for now, we go with what we 
have and can circle back if needed.


Thanks. I checked the latest webrev (moving the package description to 
package-info.java and the langtools dot test) and it all looks good to me.


-Alan