Re: RFR: 8211296: Remove HotSpot deprecation warning suppression for Mac/clang

[Kim Barrett]

> On Oct 4, 2018, at 5:25 PM, Mikael Vidstedt  
> wrote:
>> The "static build" doesn't build anymore. We're all shocked that this
>> untested configuration has bit-rotted. :)
>> 
>> LoadJavaVM uses RTLD_NOW in the normal (non-"static build") case, the
>> workaround is superfluous in that case. Given that, I'm going to
>> delete the workaround and file an RFE [1] to fix or remove the
>> currently broken "static build" support, with a comment there
>> referring to this workaround code as possibly being relevant to fixing
>> the static build.
> 
> I’m glad to see this gone. Looks good!

Thanks!

> 
> Cheers,
> Mikael
> 
>> 
>> [1] https://bugs.openjdk.java.net/browse/JDK-8211732
>> 
>> New webrev:
>> http://cr.openjdk.java.net/~kbarrett/8211296/open.01/

Re: RFR: 8211296: Remove HotSpot deprecation warning suppression for Mac/clang

[Kim Barrett]

> On Oct 4, 2018, at 5:31 PM, David Holmes  wrote:
> 
> Looks good!
> 
> Thanks for your patience and perseverance! :)

Thanks!  And thanks for your patience and perseverance too!

> 
> David

Re: RFC: JEP JDK-8208089: Implement C++14 Language Features

[Kim Barrett]

> On Oct 4, 2018, at 4:35 AM, Volker Simonis  wrote:
> 
> Hi Tim, Jeff,
> 
> Kim has assembled a quite detailed list of new C++ features intended for use 
> in HotSpot/OpenJDK (with links to the corresponding C++ standard)
> 
> https://bugs.openjdk.java.net/browse/JDK-8208089

Note that this list is intended as a starting point.  The JEP also describes a 
process for adding
to the list of permitted features, and I anticipate that process will be used.  
There are several
that I personally think would be good to add (perhaps with some limits or 
restrictions), but that
I think may involve more detailed discussion. And other folks may have their 
favorite must have
feature(s) that didn’t seem to me to be needed in this initial seed list.

Re: RFR: 8211296: Remove HotSpot deprecation warning suppression for Mac/clang

[David Holmes]

Looks good!

Thanks for your patience and perseverance! :)

David

On 5/10/2018 7:18 AM, Kim Barrett wrote:

On Oct 4, 2018, at 9:27 AM, Magnus Ihse Bursie  
wrote:

That all assumes that static build actually works at all; it doesn’t look like 
we
test that configuration these days, so who knows what bit rot may have set in.



No, we don't test static build, and never have. I'd be surprised if it's still 
possible to build a static build.

This was a feature developed for the mobile project. The static build part was 
merged, somewhat hastily, into mainline, but the rest of the mobile code is 
still lingering in the mobile project fork.

The entire "static build" concept was somewhat artificially injected in some 
parts of the build code. It is for instance not possible to build all modules with a 
static build. At the very least you need to build without java.desktop, if I remember 
correctly.

The motivation for the static build is that on iOS, apps are not allowed to 
load dynamic libraries (for some reason only Apple knows). So a developer that 
wants to build a Java app needs to have a static library version of the entire 
JDK to link with into a single executable Java launcher. So in this 
perspective, the entire use of dlopen in a static build smells funny, and might 
be just a left-over from static build being more of a hack than a properly 
supported build option.

/Magnus


The "static build" doesn't build anymore. We're all shocked that this
untested configuration has bit-rotted. :)

LoadJavaVM uses RTLD_NOW in the normal (non-"static build") case, the
workaround is superfluous in that case. Given that, I'm going to
delete the workaround and file an RFE [1] to fix or remove the
currently broken "static build" support, with a comment there
referring to this workaround code as possibly being relevant to fixing
the static build.

[1] https://bugs.openjdk.java.net/browse/JDK-8211732

New webrev:
http://cr.openjdk.java.net/~kbarrett/8211296/open.01/

Re: RFR: 8211296: Remove HotSpot deprecation warning suppression for Mac/clang

[Mikael Vidstedt]

> On Oct 4, 2018, at 2:18 PM, Kim Barrett  wrote:
> 
>> On Oct 4, 2018, at 9:27 AM, Magnus Ihse Bursie 
>>  wrote:
>>> That all assumes that static build actually works at all; it doesn’t look 
>>> like we
>>> test that configuration these days, so who knows what bit rot may have set 
>>> in.
>>> 
>> 
>> No, we don't test static build, and never have. I'd be surprised if it's 
>> still possible to build a static build. 
>> 
>> This was a feature developed for the mobile project. The static build part 
>> was merged, somewhat hastily, into mainline, but the rest of the mobile code 
>> is still lingering in the mobile project fork. 
>> 
>> The entire "static build" concept was somewhat artificially injected in some 
>> parts of the build code. It is for instance not possible to build all 
>> modules with a static build. At the very least you need to build without 
>> java.desktop, if I remember correctly. 
>> 
>> The motivation for the static build is that on iOS, apps are not allowed to 
>> load dynamic libraries (for some reason only Apple knows). So a developer 
>> that wants to build a Java app needs to have a static library version of the 
>> entire JDK to link with into a single executable Java launcher. So in this 
>> perspective, the entire use of dlopen in a static build smells funny, and 
>> might be just a left-over from static build being more of a hack than a 
>> properly supported build option. 
>> 
>> /Magnus
> 
> The "static build" doesn't build anymore. We're all shocked that this
> untested configuration has bit-rotted. :)
> 
> LoadJavaVM uses RTLD_NOW in the normal (non-"static build") case, the
> workaround is superfluous in that case. Given that, I'm going to
> delete the workaround and file an RFE [1] to fix or remove the
> currently broken "static build" support, with a comment there
> referring to this workaround code as possibly being relevant to fixing
> the static build.

I’m glad to see this gone. Looks good!

Cheers,
Mikael

> 
> [1] https://bugs.openjdk.java.net/browse/JDK-8211732
> 
> New webrev:
> http://cr.openjdk.java.net/~kbarrett/8211296/open.01/
> 

Re: RFC: JEP JDK-8208089: Implement C++14 Language Features

[Kim Barrett]

> On Oct 4, 2018, at 9:17 AM, Aleksei Voitylov  
> wrote:
> 
> Kim,
> 
> Thank you for posting this. It's an important step to make. I wanted to 
> clarify a couple of points:
> 
> 1. Since this is infrastructure JEP, is the version of JDK which will undergo 
> such changes going to be some future version or does it apply to past 
> versions as well? I'm concerned with how we can simplify security backports 
> to 8u which (I currently assume) is not subject to this change.

Future version (perhaps as soon as JDK 12).  I don't think there is
any desire to backport this change.  And yes, introducing the use of
new language features will make backports even more interesting than
they already are.  That seems unavoidable if we're going to pursue
this direction.

> 2. Which versions of GCC do you tentatively consider at this point? Non-x86 
> ports may have a problem upgrading to a specific version of GCC which the 
> shared code will use and may need additional lead time to adjust.

I think our (ad hoc) testing has been limited to gcc 7.x. But looking
at the gcc language conformance tables and release notes, gcc 5.x
might be good enough, and gcc 6.x seems fairly likely sufficient. Of
course, older versions are more likely to have bugs in some of these
new features. Updating the minimum supported versions for gcc and
clang are noted as TBD in the JEP.

Re: JEP JDK-8208089: Implement C++14 Language Features

[Kim Barrett]

> On Oct 4, 2018, at 2:59 AM, Lindenmaier, Goetz  
> wrote:
> 
> Hi,
> 
> I appreciate this is handled in a JEP and well communicated!
> 
> XLc, the compiler we use for AIX, might be a bottleneck here.
> 
> But we currently have no compiler-constraints by products on 
> our aix port of jdk12 (for jdk11 we must stay with the current 
> compiler xlc 12 which does not support C++11, and jdk11 even
> should be compilable with aCC by HP for which we won't
> get new versions!)
> 
> We will update our compiler for jdk/jdk to the most recent Xlc
> as soon as possible.
> To my current knowledge, Xlc 14 was dropped, and xlc 16
> is to be released early 2019.  It is supposed to support
> C++ 11, and also some C++ 14 features.

The information I've been able to find mostly discusses Linux support
for XLC 16.  Any information about "some C++14 features"?  I haven't
found any detail on that.

I am working toward being able to target this for JDK 12, but realize
that might not be reachable.  Windows is already there, because of
VS2017 support.  There are a couple of minor patches needed for gcc
and clang based builds; Linux-x64 and MacOSX-x64 have had a fair
amount of ad hoc testing.  We (Oracle) only switched over the
Solaris/sparc builds to the necessary compiler version (Oracle Studio
12.6) very recently, and there are some issues still to be dealt with
there.  And the currently used XLC is just not up to the change.

Re: RFR: JDK-8211073 Remove -Wno-extra from Hotspot

[Kim Barrett]

> On Oct 4, 2018, at 9:14 AM, Magnus Ihse Bursie 
>  wrote:
> 
> 3 okt. 2018 kl. 00:18 skrev Kim Barrett :
> 
>> I went through all the associated warnings for gcc7.3 and categorized
>> them.  Some are just not interesting or relevant to us.  I think these
>> include
>> 
>> […]
> 
> I agree that some, like clobbered, is not relevant (but I think that also 
> only applies to C code..?). In general, it doesn't hurt though, to turn on 
> warnings that are unlikely to be triggered. I realize it also helps little to 
> nothing, but once again, if this is the price we have to pay for catching 
> some real problems, it's a small price indeed. 

I made this list for completeness of discussion.  I think these
options should carry no weight and should just be ignored in the
decision making process.

> OTOH, I don't really agree that old-style-declaration is irrelevant. At least 
> in the JDK libraries there are multiple places where non-prototype 
> declarations are made, and apart from the style issue, it's a bug waiting to 
> happen if the function change its signature. I don't remember if this is the 
> case in Hotspot too, but it wouldn't surprise me, if we don't check for it. 

Regarding -Wold-style-declaration, remember that we're discussion a
change to HotSpot, where this isn't relevant.

> empty-body is mostly good at catching style issues, like double semicolons. 
> It's not a big deal, but it also never hurts to keep up good code quality. I 
> have not seen it make any false positives.

-Wempty-body doesn't catch empty semi-colons (which are permitted in
C++11 and later, and no compiler I know of complains about them in
C++98 mode, because they arise too often with conditional compilation
and macros and such).

There is also a well-known macro idiom involving empty if bodies, e.g.

  if (!) {} else 

to avoid dangling else.  Since you didn't run into any occurences of
this warning, we're probably not using that idiom right now.  But
there's nothing wrong with it and I've used it myself in the past.  It
could have been used in some of the UL macros, but a different idiom
was used there.  (The two are semantically mostly equivalent, the
differences being fairly subtle.  If I'd written these macros they
would probably be using the "if" style.)

> shift-negative-values is a bit beyond me, but as I understand it, it warns 
> about a behavior that has changed recently, or will be changed, in the 
> standards, to become undefined. That sounds to me like something that it 
> would be better to act preemptively on. But then again, I might have 
> misunderstood things. If so, we can definitely turn it off. 

I mis-remembered the situation with -Wshift-negative-value; we
probably do want that turned on once we've eliminated uses (and there
are some).  C89/C++98 didn't specify the behavior in that case.
C99/C++11 clarified that it is explicitly UB.


>> -Wimplicit-fallthrough=3 requires a structured comment or C++17
>> feature to quiet the warning in code that is using the feature.
>> Provides some benefit.  I'm surprised there aren't any warnings from
>> this. Intentional fall-through is a pretty common idiom.
> 
> At level 3, the "structured" comment is just something like a line like this:
> 
> // fallthrough
> 
> Most, maybe all, fallthroughs in the hotspot code already does that. I think 
> it's a reasonable requirement to show that the fallthrough is intentional. If 
> you think it's too strong, we can lower it to level 1, which just requires a 
> comment, with whatever text (or even empty, I think).

I'm concerned that other compilers / checkers might want other
comments, and one might end up with a bit of a mess because of that.
If Visual Studio (for example) has a similar warning option, we might
want to turn that on too.

>> I think the benefit we get from detecting future occurrences of the
>> copy constructor bug is not really worth the current cost of "fixing"
>> the occurrences of conditional expressions with enumerators.  I might
>> feel differently if a pass were made through those to replace uses of
>> enum with static const members.
> 
> So I think this is what it really boils down to. Just to summarize/repeat 
> what you said, so I'm sure we understand each other correctly:
> 
> There's an upside to enabling Wextra; we know already that it found some real 
> bugs, but the real benefit is all future potential bugs it will prohibit, and 
> that's hard to quantify. On the other hand, there's a downside; and that's 
> the conditional enum warnings, whose fixes are ugly. But, if the code was 
> cleaned up, so those enum fixes were no longer ugly, then it would be a net 
> positive to turn on Wextra.
> 
> I think that's a reasonable solution for now. If we agree on this, I'll open 
> a new bug with just the copy constructor fixes, and a enhancement request for 
> replacing the enums with static const, and making the current bug ("turn on 
> Wextra") dependent on that. 

That seems like a good plan to me.

Re: RFR: JDK-8211677: Java resource copy and clean should use MakeTargetDir macro

[Erik Joelsson]

Hello,

On 2018-10-04 04:47, Magnus Ihse Bursie wrote:

Looks good to me.

Have you searched the code for more instances of mkdir -p?

Just did, lots of places. I will create a followup issue for this. The 
current issue is failing too many builds in our CI to not be fixed fast.
Also, maybe we should check if using AC_PROG_MKDIR_P would give us a 
better option on Solaris. (See 
https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.69/html_node/Particular-Programs.html#Particular-Programs). 
It is supposed to give a non-race prone version of mkdir -p.


Just to be clear: Even if we start using it for MKDIR_P, I still think 
it's a good idea to use our macro.


That's certainly interesting and should be investigated. I will add it 
to the followup issue.


/Erik

/Magnus

4 okt. 2018 kl. 00:51 skrev Erik Joelsson >:


Since upgrading our Solaris build machines to 11.3, we have 
experienced intermittent build failures in the recipes copying java 
resource files. It's unclear why this started happening now after 
having worked fine for so long, but it seems it's a race caused by 
concurrent calls to "mkdir -p". In other recipes we have worked 
around this by using a macro "MakeDir" which reduces the likelihood 
of concurrent calls happening. Rewriting these copy and clean rules 
to use the current preferred macros seem to alleviate the problem for 
us on Solaris 11.3, and also makes the build a little bit more coherent.


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

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



/Erik

Re: RFR: 8210459: Add support for generating compile_commands.json

[Erik Joelsson]

Looks good.

Minor style issue, the broken find line should be indented 4 spaces for 
continuation. Strictly speaking so should the awk program lines be in 
this case. No need for respin.


/Erik


On 2018-10-04 06:31, Magnus Ihse Bursie wrote:

4 okt. 2018 kl. 15:03 skrev Robin Westberg :

Hi Magnus,

Thanks again for reviewing!


On 4 Oct 2018, at 13:38, Magnus Ihse Bursie  
wrote:

Webrev (full): http://cr.openjdk.java.net/~rwestberg/8210459/webrev.04/

I was about to say that this looks good, but then I discovered a weird thing. I'm the SED 
line, line 50, there is a weird C-like Unicode character instead of a quote, after -e. 
Looks really odd. Is it a "smart quote" gone wrong? Can you look into it and 
fix it? You don't need to respin the webrev for that. Otherwise, you're all good to go.

Ah indeed, nice catch! Fortunately it seems to be an issue with the “git-webrev” 
tool, there’s a missing semicolon in the html’ized version: $(SED) -e 
s/^/[. (The ‘raw’ view for example is correct).

Good.


I did notice one more thing that I was hoping to sneak into an in-place update: 
the raw ‘cat’ on line 47 should probably be $(CAT) as well, right? I’ll sanity 
check it with that change to be certain.

Yeah, it should. Shame on me and Erik for not catching it. ;-)

No need to respin.

/Magnus


Best regards,
Robin


/Magnus


Webrev (incremental): 
http://cr.openjdk.java.net/~rwestberg/8210459/webrev.03-04/

Best regards,
Robin


Otherwise I think this is good now.

/Erik

The AWT constructs also confuses me. Maybe you can expand a bit on the comment, 
because it really is non-trivial. You are executing a cp for each tmpfile you 
find? But what if you find multiple tmpfiles? There could certainly be multiple 
commands using @ constructs?

What it does it actually to invoke fixpath on everything inside the final 
compile_commands.json file, but in order to make fixpath do that, it presents 
the compile_commands.json file as an @-argument. Fixpath will then translate 
that argument to a temporary filename containing corrected paths, and that’s 
the file we save (since it’s deleted when the invoked command terminates). I’ve 
updated the comment a bit, hopefully it’s more clear now.

Another option would be to extend the fixpath utility itself to support some 
additional argument to just do inline path correction on a given file, but I 
felt that it would be a more risky change.


* In make/Main.gmk, you can just do "($(CD) $(OUTPUTDIR) && $(RM) -r build*.log* 
compile_commands.json)" on a single line.

Fixed.


* In make/common/MakeBase.gmk: I'd prefer if these functions were move to 
make/common/JdkNativeCompilation.gmk, close to the FindSrcDirsForLib etc 
definitions.

Fixed.


* In make/common/NativeCompilation.gmk, I'm not entirely happy with the 
$1_OBJ_COMMON_OPTIONS_PRE/POST construct. I understand what you are trying to 
achieve, but I think this gets very hard to read. I don't have a perfect 
solution in mind. But perhaps something like this:
$1_DEPS_OPTIONS := $$($1_DEP_FLAG) $$($1_DEP).tmp
$1_COMPILE_COMMAND := $$($1_COMPILER) $$($1_FLAGS) $$($1_DEPS_OPTIONS) 
$(CC_OUT_OPTION)$$($1_OBJ) $$($1_SRC_FILE))

and for the compile commands, use $$(filter-out $$($1_DEPS_OPTIONS), 
$$($1_COMPILE_COMMAND)).

Perhaps some unification with the Microsoft toolchain is possible by setting 
$1_DEPS_OPTIONS to -showIncludes.


An alternative, perhaps better, is to move the deps flag to the start. Then you 
could do something like the above, but set

$1_COMPILE_OPTIONS := $$($1_FLAGS) $(CC_OUT_OPTION)$$($1_OBJ) $$($1_SRC_FILE))
and when compiling do this:
$$($1_COMPILER)  $$($1_DEPS_OPTIONS)  $$($1_COMPILE_OPTIONS)

That would get rid of the filter-out, keep duplication low but still be 
readable.

This assumes that ordering for the deps option is irrelevant. I think it is but 
it would have to be tested.

Sounds good, fixed.


/Magnus


Erik, what do you think?

* In make/lib/Lib-jdk.accessibility.gmk, this seems double incorrect. You are 
missing a libjawt path segment. And you wanted to use FindStaticLib.

Ah, good catch, I suspect it still works as the jawt.lib file in the 
modules_lib folder is dependent on jawt.lib in the native folder. Fixed.


Overall, I believe we're misusing the "static lib" concept on Windows. The .lib files are 
not static libs, the way we mean it on unixes. Instead, the lib files is some kind of metadata on 
dll that are used when linking with those dlls. So we should introduce a better concept for these, 
and maybe something like FindLibDescriptor (or whatever). That should not really be part of this 
fix, though, so for the moment I'm going to accept that we call these "static libs" on 
Windows.

This also makes me wonder how much testing this patch has recieved? I know a 
broken dependency in the worst case only shows up as a race, but have you 
verified it thoroughly even on Windows? And even without compile_commands?

What I’ve been doing, apart from making sure tier1 tests and 

RE: [PING] [8u] RFR: 8073139: PPC64: User-visible arch directory and os.arch value on ppc64le cause issues with Java tooling

[Lindenmaier, Goetz]

Thanks!

I'm fine for this to go to 8.

Best regards,
  Goetz.

> -Original Message-
> From: Severin Gehwolf 
> Sent: Donnerstag, 4. Oktober 2018 16:40
> To: Lindenmaier, Goetz ; Erik Joelsson
> ; hotspot-dev  d...@openjdk.java.net>; ppc-aix-port-dev  d...@openjdk.java.net>; build-dev 
> Subject: Re: [PING] [8u] RFR: 8073139: PPC64: User-visible arch directory and
> os.arch value on ppc64le cause issues with Java tooling
> 
> Hi Goetz,
> 
> On Tue, 2018-10-02 at 10:40 +, Lindenmaier, Goetz wrote:
> [...]
> > Did you test this on ppc64 be, too?
> 
> I've tested this patch on PPC64 BE Linux now too. There is no change
> (as expected):
> 
> $ ./jdk8-ppc64be-after/bin/java TestProperty
> os.arch == ppc64
> java.library.path ==
> /usr/java/packages/lib/ppc64:/usr/lib64:/lib64:/lib:/usr/lib
> $ ./jdk8-ppc64be-before/bin/java TestProperty
> os.arch == ppc64
> java.library.path ==
> /usr/java/packages/lib/ppc64:/usr/lib64:/lib64:/lib:/usr/lib
> 
> Thanks,
> Severin
> 
> > Best regards,
> >   Goetz.
> >
> > > -Original Message-
> > > From: ppc-aix-port-dev 
> On
> > > Behalf Of Severin Gehwolf
> > > Sent: Dienstag, 2. Oktober 2018 12:34
> > > To: Erik Joelsson ; hotspot-dev  > > d...@openjdk.java.net>; ppc-aix-port-dev  > > d...@openjdk.java.net>; build-dev 
> > > Subject: Re: [PING] [8u] RFR: 8073139: PPC64: User-visible arch directory
> and
> > > os.arch value on ppc64le cause issues with Java tooling
> > >
> > > Hi,
> > >
> > > Pinging PPC porters. Does this look reasonable to you?
> > >
> > > Thanks,
> > > Severin
> > >
> > >
> > > On Fri, 2018-09-28 at 08:56 -0700, Erik Joelsson wrote:
> > > > Build changes look ok to me.
> > > >
> > > > /Erik
> > > >
> > > >
> > > > On 2018-09-26 04:26, Severin Gehwolf wrote:
> > > > > Hi,
> > > > >
> > > > > Could I please get reviews for this JDK 8 backport which fixes some
> > > > > tooling issues on Linux ppc64le? Prior this patch, a ppc64le build
> > > > > would report as "ppc64" via os.arch system property which breaks
> > > > > tooling such as maven in as much as if some dependency needs
> native
> > > > > libraries it would download BE binaries where it actually should
> > > > > download LE binaries. Example for os.arch/java.library.path:
> > > > >
> > > > > pre:
> > > > > $ ./jdk8-pre-ppc64le/bin/java TestProperty
> > > > > java.library.path =
> > >
> > > /usr/java/packages/lib/ppc64:/usr/lib64:/lib64:/lib:/usr/lib
> > > > > os.arch = ppc64
> > > > >
> > > > > post:
> > > > > $ ./jdk8-post-ppc64le/bin/java TestProperty
> > > > > java.library.path =
> > >
> > > /usr/java/packages/lib/ppc64le:/usr/lib64:/lib64:/lib:/usr/lib
> > > > > os.arch = ppc64le
> > > > >
> > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8073139
> > > > > webrevs: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-
> > >
> > > 8073139/jdk8/01/
> > > > >
> > > > > Including build-dev for build changes. hotspot-dev and ppc-aix-port-
> dev
> > > > > for JDK/hotspot changes.
> > > > >
> > > > > This backport should only have minor differences to the changes in
> JDK
> > > > > 11. We have been using similar patches in Fedora for months.
> Thoughts?
> > > > >
> > > > > Thanks,
> > > > > Severin
> > > > >
> > > >
> > > >
> >
> >

Re: [PING] [8u] RFR: 8073139: PPC64: User-visible arch directory and os.arch value on ppc64le cause issues with Java tooling

[Severin Gehwolf]

Hi Goetz,

On Tue, 2018-10-02 at 10:40 +, Lindenmaier, Goetz wrote:
[...]
> Did you test this on ppc64 be, too?  

I've tested this patch on PPC64 BE Linux now too. There is no change
(as expected):

$ ./jdk8-ppc64be-after/bin/java TestProperty
os.arch == ppc64
java.library.path == 
/usr/java/packages/lib/ppc64:/usr/lib64:/lib64:/lib:/usr/lib
$ ./jdk8-ppc64be-before/bin/java TestProperty
os.arch == ppc64
java.library.path == 
/usr/java/packages/lib/ppc64:/usr/lib64:/lib64:/lib:/usr/lib

Thanks,
Severin

> Best regards,
>   Goetz.
> 
> > -Original Message-
> > From: ppc-aix-port-dev  On
> > Behalf Of Severin Gehwolf
> > Sent: Dienstag, 2. Oktober 2018 12:34
> > To: Erik Joelsson ; hotspot-dev  > d...@openjdk.java.net>; ppc-aix-port-dev  > d...@openjdk.java.net>; build-dev 
> > Subject: Re: [PING] [8u] RFR: 8073139: PPC64: User-visible arch directory 
> > and
> > os.arch value on ppc64le cause issues with Java tooling
> > 
> > Hi,
> > 
> > Pinging PPC porters. Does this look reasonable to you?
> > 
> > Thanks,
> > Severin
> > 
> > 
> > On Fri, 2018-09-28 at 08:56 -0700, Erik Joelsson wrote:
> > > Build changes look ok to me.
> > > 
> > > /Erik
> > > 
> > > 
> > > On 2018-09-26 04:26, Severin Gehwolf wrote:
> > > > Hi,
> > > > 
> > > > Could I please get reviews for this JDK 8 backport which fixes some
> > > > tooling issues on Linux ppc64le? Prior this patch, a ppc64le build
> > > > would report as "ppc64" via os.arch system property which breaks
> > > > tooling such as maven in as much as if some dependency needs native
> > > > libraries it would download BE binaries where it actually should
> > > > download LE binaries. Example for os.arch/java.library.path:
> > > > 
> > > > pre:
> > > > $ ./jdk8-pre-ppc64le/bin/java TestProperty
> > > > java.library.path =
> > 
> > /usr/java/packages/lib/ppc64:/usr/lib64:/lib64:/lib:/usr/lib
> > > > os.arch = ppc64
> > > > 
> > > > post:
> > > > $ ./jdk8-post-ppc64le/bin/java TestProperty
> > > > java.library.path =
> > 
> > /usr/java/packages/lib/ppc64le:/usr/lib64:/lib64:/lib:/usr/lib
> > > > os.arch = ppc64le
> > > > 
> > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8073139
> > > > webrevs: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-
> > 
> > 8073139/jdk8/01/
> > > > 
> > > > Including build-dev for build changes. hotspot-dev and ppc-aix-port-dev
> > > > for JDK/hotspot changes.
> > > > 
> > > > This backport should only have minor differences to the changes in JDK
> > > > 11. We have been using similar patches in Fedora for months. Thoughts?
> > > > 
> > > > Thanks,
> > > > Severin
> > > > 
> > > 
> > > 
> 
> 

Re: RFR: 8210459: Add support for generating compile_commands.json

[Magnus Ihse Bursie]

> 4 okt. 2018 kl. 15:03 skrev Robin Westberg :
> 
> Hi Magnus,
> 
> Thanks again for reviewing!
> 
>>> On 4 Oct 2018, at 13:38, Magnus Ihse Bursie  
>>> wrote:
>>> 
>>> Webrev (full): http://cr.openjdk.java.net/~rwestberg/8210459/webrev.04/
>> 
>> I was about to say that this looks good, but then I discovered a weird 
>> thing. I'm the SED line, line 50, there is a weird C-like Unicode character 
>> instead of a quote, after -e. Looks really odd. Is it a "smart quote" gone 
>> wrong? Can you look into it and fix it? You don't need to respin the webrev 
>> for that. Otherwise, you're all good to go. 
> 
> Ah indeed, nice catch! Fortunately it seems to be an issue with the 
> “git-webrev” tool, there’s a missing semicolon in the html’ized version: 
> $(SED) -e s/^/[. (The ‘raw’ view for example is correct).

Good.

> 
> I did notice one more thing that I was hoping to sneak into an in-place 
> update: the raw ‘cat’ on line 47 should probably be $(CAT) as well, right? 
> I’ll sanity check it with that change to be certain.

Yeah, it should. Shame on me and Erik for not catching it. ;-)

No need to respin.

/Magnus 

> 
> Best regards,
> Robin
> 
>> /Magnus
>> 
>>> Webrev (incremental): 
>>> http://cr.openjdk.java.net/~rwestberg/8210459/webrev.03-04/
>>> 
>>> Best regards,
>>> Robin
>>> 
 
 Otherwise I think this is good now.
 
 /Erik
>>> The AWT constructs also confuses me. Maybe you can expand a bit on the 
>>> comment, because it really is non-trivial. You are executing a cp for 
>>> each tmpfile you find? But what if you find multiple tmpfiles? There 
>>> could certainly be multiple commands using @ constructs?
> What it does it actually to invoke fixpath on everything inside the final 
> compile_commands.json file, but in order to make fixpath do that, it 
> presents the compile_commands.json file as an @-argument. Fixpath will 
> then translate that argument to a temporary filename containing corrected 
> paths, and that’s the file we save (since it’s deleted when the invoked 
> command terminates). I’ve updated the comment a bit, hopefully it’s more 
> clear now.
> 
> Another option would be to extend the fixpath utility itself to support 
> some additional argument to just do inline path correction on a given 
> file, but I felt that it would be a more risky change.
> 
>>> * In make/Main.gmk, you can just do "($(CD) $(OUTPUTDIR) && $(RM) -r 
>>> build*.log* compile_commands.json)" on a single line.
> Fixed.
> 
>>> * In make/common/MakeBase.gmk: I'd prefer if these functions were move 
>>> to make/common/JdkNativeCompilation.gmk, close to the FindSrcDirsForLib 
>>> etc definitions.
> Fixed.
> 
>>> * In make/common/NativeCompilation.gmk, I'm not entirely happy with the 
>>> $1_OBJ_COMMON_OPTIONS_PRE/POST construct. I understand what you are 
>>> trying to achieve, but I think this gets very hard to read. I don't 
>>> have a perfect solution in mind. But perhaps something like this:
>>> $1_DEPS_OPTIONS := $$($1_DEP_FLAG) $$($1_DEP).tmp
>>> $1_COMPILE_COMMAND := $$($1_COMPILER) $$($1_FLAGS) $$($1_DEPS_OPTIONS) 
>>> $(CC_OUT_OPTION)$$($1_OBJ) $$($1_SRC_FILE))
>>> 
>>> and for the compile commands, use $$(filter-out $$($1_DEPS_OPTIONS), 
>>> $$($1_COMPILE_COMMAND)).
>>> 
>>> Perhaps some unification with the Microsoft toolchain is possible by 
>>> setting $1_DEPS_OPTIONS to -showIncludes.
>>> 
>> An alternative, perhaps better, is to move the deps flag to the start. 
>> Then you could do something like the above, but set
>> 
>> $1_COMPILE_OPTIONS := $$($1_FLAGS) $(CC_OUT_OPTION)$$($1_OBJ) 
>> $$($1_SRC_FILE))
>> and when compiling do this:
>> $$($1_COMPILER)  $$($1_DEPS_OPTIONS)  $$($1_COMPILE_OPTIONS)
>> 
>> That would get rid of the filter-out, keep duplication low but still be 
>> readable.
>> 
>> This assumes that ordering for the deps option is irrelevant. I think it 
>> is but it would have to be tested.
> Sounds good, fixed.
> 
>> /Magnus
>> 
>>> Erik, what do you think?
>>> 
>>> * In make/lib/Lib-jdk.accessibility.gmk, this seems double incorrect. 
>>> You are missing a libjawt path segment. And you wanted to use 
>>> FindStaticLib.
> Ah, good catch, I suspect it still works as the jawt.lib file in the 
> modules_lib folder is dependent on jawt.lib in the native folder. Fixed.
> 
>>> Overall, I believe we're misusing the "static lib" concept on Windows. 
>>> The .lib files are not static libs, the way we mean it on unixes. 
>>> Instead, the lib files is some kind of metadata on dll that are used 
>>> when linking with those dlls. So we should introduce a better concept 
>>> for these, and maybe something like FindLibDescriptor (or whatever). 
>>> That should not really be part of this fix, though, so for 

Re: RFR: 8211296: Remove HotSpot deprecation warning suppression for Mac/clang

[Magnus Ihse Bursie]

3 okt. 2018 kl. 01:30 skrev Kim Barrett :

>> On Oct 2, 2018, at 6:10 PM, David Holmes  wrote:
>> The archaeology is getting too hard. :(
> 
> No kidding!
> 
>> Yes but you will need to go through a full round of testing, not just tier 
>> 1-3, before being reasonably confident the workaround is not needed. I'd 
>> hate to see this break things "in the wild" just because we don't have 
>> adequate test coverage.
>> 
>> Other opinions welcomed.
> 
> I was planning to hit it with tier1-8 and possibly other things if I can find 
> them.
> JCK tests too; I saw some instructions for running those in mach5 recently.
> 
> That all assumes that static build actually works at all; it doesn’t look 
> like we
> test that configuration these days, so who knows what bit rot may have set in.
> 

No, we don't test static build, and never have. I'd be surprised if it's still 
possible to build a static build. 

This was a feature developed for the mobile project. The static build part was 
merged, somewhat hastily, into mainline, but the rest of the mobile code is 
still lingering in the mobile project fork. 

The entire "static build" concept was somewhat artificially injected in some 
parts of the build code. It is for instance not possible to build all modules 
with a static build. At the very least you need to build without java.desktop, 
if I remember correctly. 

The motivation for the static build is that on iOS, apps are not allowed to 
load dynamic libraries (for some reason only Apple knows). So a developer that 
wants to build a Java app needs to have a static library version of the entire 
JDK to link with into a single executable Java launcher. So in this 
perspective, the entire use of dlopen in a static build smells funny, and might 
be just a left-over from static build being more of a hack than a properly 
supported build option. 

/Magnus

Re: RFC: JEP JDK-8208089: Implement C++14 Language Features

[Aleksei Voitylov]

Kim,

Thank you for posting this. It's an important step to make. I wanted to 
clarify a couple of points:


1. Since this is infrastructure JEP, is the version of JDK which will 
undergo such changes going to be some future version or does it apply to 
past versions as well? I'm concerned with how we can simplify security 
backports to 8u which (I currently assume) is not subject to this change.


2. Which versions of GCC do you tentatively consider at this point? 
Non-x86 ports may have a problem upgrading to a specific version of GCC 
which the shared code will use and may need additional lead time to adjust.


Thanks,

-Aleksei


On 03/10/2018 22:13, Kim Barrett wrote:

I've submitted a JEP for

(1) enabling the use of C++14 Language Features when building the JDK,

(2) define a process for deciding and documenting which new features
can be used or are forbidden in HotSpot code,

(3) provide an initial list of permitted and forbidden new features.

https://bugs.openjdk.java.net/browse/JDK-8208089

Re: RFR: JDK-8211073 Remove -Wno-extra from Hotspot

[Magnus Ihse Bursie]

Kim,

Thank you for your detailed analysis! Comments inline...

3 okt. 2018 kl. 00:18 skrev Kim Barrett :

>> On Sep 29, 2018, at 1:36 PM, Magnus Ihse Bursie 
>>  wrote:
>> 
>> On 2018-09-28 23:22, Kim Barrett wrote:
>> 
 On Sep 24, 2018, at 4:31 PM, Magnus Ihse Bursie 
  wrote:
 The second warning about the copy constructor is, for what I can tell, a 
 highly valid warning and the code it warned on was indeed broken. As far 
 as I can tell, in a derived copy constructor you should always explicitly 
 initialize the base class.
>>> I agree the copy constructor warnings are correct and indicate potentially 
>>> serious bugs.
>>> These copy constructor changes look correct to me.
>>> 
>>> The code that is being missed because of this bug is debug-only usage 
>>> verification.  I think
>>> bad things might happen if we C-heap allocated a ResourceObj and then 
>>> copied that object.
>>> Maybe we fortuitously don’t ever do that?
>> If we had triggered problems we'd probably found out the issue by now, so 
>> its likely we're not doing anything that causes this to happen currently. 
>> But latent bugs like these are scary, and unnecessary, especially if we can 
>> get the compiler's help to avoid them.
>>> It’s unfortunate that the only way to get these warnings from gcc seems to 
>>> be via -Wextra.
>> I agree, it's unfortunate. -Wextra in gcc actually means two things: a bunch 
>> of "extra" warnings, that you can turn on or off as a group only by enabling 
>> or disabling -Wextra, and a set of separate warnings that this option also 
>> turns on by default. The latter is more of a convenience to get a set of 
>> warnings that the gcc authors recommend for more in-depth warnings. Since 
>> they can be individually disabled, we don't need to care much about them.
>> 
>> In regard to your previous mail, there has not been much change in the scope 
>> of -Wextra. Between gcc 4.8 and 7.3, no changes were made in the first part 
>> (the "extra" warnings), and only four more warnings were added to the second 
>> part (-Wcast-function-type, -Wimplicit-fallthrough=3, -Wredundant-move and 
>> -Wshift-negative-value), all of which can be turned off if we don't want 
>> them.
>> 
>> In general, we try to make the product compile without warnings on common 
>> platforms, but as you say, unless you use one of the compilers that are 
>> regularly used (at Oracle or elsewhere), there's a risk of triggering new 
>> warnings. However, using configure --disable-warnings-as-errors makes the 
>> build pass anyway, and is already commonly in use by those building on more 
>> exotic platforms or compiler versions.
>> 
>> I would have preferred to have individual warnings to enable, but gcc has 
>> not moved all warnings from -Wextra to individual warnings (new warnings, 
>> though, have all been given individual names). Since the warnings, as you 
>> agree, can find actual bugs, I think it's worth the hassle to enable 
>> -Wextra. (We already do that for all native code in OpenJDK, except hotspot, 
>> btw.)
> 
> gcc8.2 adds -Wcast-function-type, which *might* cause us problems.  We
> play some interesting games with function types in some places, and I
> don't know if they'll trigger this.  We can turn it off explicitly if
> it is a problem (or perhaps preemptively).
> 
> I went through all the associated warnings for gcc7.3 and categorized
> them.  Some are just not interesting or relevant to us.  I think these
> include
> 
> -Wclobbered
> -Wignored-qualifiers
> -Wmissing-field-initializers 
> -Wmissing-parameter-type
> -Wold-style-declaration
> -Woverride-init 
> Ambiguous virtual bases.
> Subscripting an array that has been declared register
> Taking address of a variable declared register

I agree that some, like clobbered, is not relevant (but I think that also only 
applies to C code..?). In general, it doesn't hurt though, to turn on warnings 
that are unlikely to be triggered. I realize it also helps little to nothing, 
but once again, if this is the price we have to pay for catching some real 
problems, it's a small price indeed. 

OTOH, I don't really agree that old-style-declaration is irrelevant. At least 
in the JDK libraries there are multiple places where non-prototype declarations 
are made, and apart from the style issue, it's a bug waiting to happen if the 
function change its signature. I don't remember if this is the case in Hotspot 
too, but it wouldn't surprise me, if we don't check for it. 

> Some we are already using:
> -Wsign-compiler
> -Wtype-limits
> -Wuninitialized
> 
> Some I think we should never enable (so need to explicitly disable if
> adding -Wextra):
> -Wempty-body
> -Wshift-negative-value
> -Wunused-parameter

I agree that unused-parameter is worthless. I think we should disable it 
globally, if that's not already done. 

empty-body is mostly good at catching style issues, like double semicolons. 
It's not a big deal, but it also never hurts to keep up good code 

Re: RFR: 8210459: Add support for generating compile_commands.json

[Robin Westberg]

Hi Magnus,

Thanks again for reviewing!

> On 4 Oct 2018, at 13:38, Magnus Ihse Bursie  
> wrote:
> 
>> Webrev (full): http://cr.openjdk.java.net/~rwestberg/8210459/webrev.04/
> 
> I was about to say that this looks good, but then I discovered a weird thing. 
> I'm the SED line, line 50, there is a weird C-like Unicode character instead 
> of a quote, after -e. Looks really odd. Is it a "smart quote" gone wrong? Can 
> you look into it and fix it? You don't need to respin the webrev for that. 
> Otherwise, you're all good to go. 

Ah indeed, nice catch! Fortunately it seems to be an issue with the 
“git-webrev” tool, there’s a missing semicolon in the html’ized version: $(SED) 
-e s/^/[. (The ‘raw’ view for example is correct).

I did notice one more thing that I was hoping to sneak into an in-place update: 
the raw ‘cat’ on line 47 should probably be $(CAT) as well, right? I’ll sanity 
check it with that change to be certain.

Best regards,
Robin

> /Magnus
> 
>> Webrev (incremental): 
>> http://cr.openjdk.java.net/~rwestberg/8210459/webrev.03-04/
>> 
>> Best regards,
>> Robin
>> 
>>> 
>>> Otherwise I think this is good now.
>>> 
>>> /Erik
>> The AWT constructs also confuses me. Maybe you can expand a bit on the 
>> comment, because it really is non-trivial. You are executing a cp for 
>> each tmpfile you find? But what if you find multiple tmpfiles? There 
>> could certainly be multiple commands using @ constructs?
 What it does it actually to invoke fixpath on everything inside the final 
 compile_commands.json file, but in order to make fixpath do that, it 
 presents the compile_commands.json file as an @-argument. Fixpath will 
 then translate that argument to a temporary filename containing corrected 
 paths, and that’s the file we save (since it’s deleted when the invoked 
 command terminates). I’ve updated the comment a bit, hopefully it’s more 
 clear now.
 
 Another option would be to extend the fixpath utility itself to support 
 some additional argument to just do inline path correction on a given 
 file, but I felt that it would be a more risky change.
 
>> * In make/Main.gmk, you can just do "($(CD) $(OUTPUTDIR) && $(RM) -r 
>> build*.log* compile_commands.json)" on a single line.
 Fixed.
 
>> * In make/common/MakeBase.gmk: I'd prefer if these functions were move 
>> to make/common/JdkNativeCompilation.gmk, close to the FindSrcDirsForLib 
>> etc definitions.
 Fixed.
 
>> * In make/common/NativeCompilation.gmk, I'm not entirely happy with the 
>> $1_OBJ_COMMON_OPTIONS_PRE/POST construct. I understand what you are 
>> trying to achieve, but I think this gets very hard to read. I don't have 
>> a perfect solution in mind. But perhaps something like this:
>> $1_DEPS_OPTIONS := $$($1_DEP_FLAG) $$($1_DEP).tmp
>> $1_COMPILE_COMMAND := $$($1_COMPILER) $$($1_FLAGS) $$($1_DEPS_OPTIONS) 
>> $(CC_OUT_OPTION)$$($1_OBJ) $$($1_SRC_FILE))
>> 
>> and for the compile commands, use $$(filter-out $$($1_DEPS_OPTIONS), 
>> $$($1_COMPILE_COMMAND)).
>> 
>> Perhaps some unification with the Microsoft toolchain is possible by 
>> setting $1_DEPS_OPTIONS to -showIncludes.
>> 
> An alternative, perhaps better, is to move the deps flag to the start. 
> Then you could do something like the above, but set
> 
> $1_COMPILE_OPTIONS := $$($1_FLAGS) $(CC_OUT_OPTION)$$($1_OBJ) 
> $$($1_SRC_FILE))
> and when compiling do this:
> $$($1_COMPILER)  $$($1_DEPS_OPTIONS)  $$($1_COMPILE_OPTIONS)
> 
> That would get rid of the filter-out, keep duplication low but still be 
> readable.
> 
> This assumes that ordering for the deps option is irrelevant. I think it 
> is but it would have to be tested.
 Sounds good, fixed.
 
> /Magnus
> 
>> Erik, what do you think?
>> 
>> * In make/lib/Lib-jdk.accessibility.gmk, this seems double incorrect. 
>> You are missing a libjawt path segment. And you wanted to use 
>> FindStaticLib.
 Ah, good catch, I suspect it still works as the jawt.lib file in the 
 modules_lib folder is dependent on jawt.lib in the native folder. Fixed.
 
>> Overall, I believe we're misusing the "static lib" concept on Windows. 
>> The .lib files are not static libs, the way we mean it on unixes. 
>> Instead, the lib files is some kind of metadata on dll that are used 
>> when linking with those dlls. So we should introduce a better concept 
>> for these, and maybe something like FindLibDescriptor (or whatever). 
>> That should not really be part of this fix, though, so for the moment 
>> I'm going to accept that we call these "static libs" on Windows.
>> 
>> This also makes me wonder how much testing this patch has recieved? I 
>> know a broken dependency in the worst case only shows up as a race, but 
>> have you verified it 

Re: RFC: JEP JDK-8208089: Implement C++14 Language Features

[Magnus Ihse Bursie]

> 4 okt. 2018 kl. 10:57 skrev Martijn Verburg :
> 
> Hi Kim,
> 
> I like this initiative.  I'm wondering if some of these rules can be easily
> codified or written into a jcheck style checker (ccheck?) so that Authors
> can adhere to the conventions and not rely on a Human review to pick out
> where that convention isn't met.

That's an interesting thought!

I googled around a bit, but could find no obvious candidate for doing such a 
check. In fact, parsing and analyzing C++ seems quite a hard problem, compared 
to many other languages. (Sad, but not really surprising.)

I found two possible routes to explore: cpplint [1], the official Google tool 
to verify that the Google C++ Style Guide [2] is followed, and Vera++ [3], a 
framework for creating scripts that can analyze and/or transform C++ code.

Neither seem like an ideal solution. The Google tool is hard coded to support 
the Google rules. It's possible we can fork it and add our own, but it's not 
clear that this is even possible, much less so that it's a good way forward. 
Vera++, on the other hand, seems much more likely to be able to provide the 
tooling needed to write checks that enforce these rules. However, what we have 
is just a framework, and someone's got to write those rules (in TCL of all 
languages...). Then again, Vera++ is an old and quite popular tool, so its 
possible there is already a bunch of predefined rules that we can use as a 
starting point out there. (I didn't go that far in my analysis).

Apart from these two, there appear to be no popular, well-encompassing, open 
source C++ checker out there, that could possibly be verifying rules like 
these. :(

/Magnus

[1] https://github.com/google/styleguide/tree/gh-pages/cpplint
[2] https://google.github.io/styleguide/cppguide.html
[3] https://bitbucket.org/verateam/vera/wiki/Introduction

> 
> Cheers,
> Martijn
> 
> 
>> On Wed, 3 Oct 2018 at 20:13, Kim Barrett  wrote:
>> 
>> I've submitted a JEP for
>> 
>> (1) enabling the use of C++14 Language Features when building the JDK,
>> 
>> (2) define a process for deciding and documenting which new features
>> can be used or are forbidden in HotSpot code,
>> 
>> (3) provide an initial list of permitted and forbidden new features.
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8208089
>> 
>> 

Re: RFR: JDK-8211677: Java resource copy and clean should use MakeTargetDir macro

[Magnus Ihse Bursie]

Looks good to me. 

Have you searched the code for more instances of mkdir -p?

Also, maybe we should check if using AC_PROG_MKDIR_P would give us a better 
option on Solaris. (See 
https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.69/html_node/Particular-Programs.html#Particular-Programs).
 It is supposed to give a non-race prone version of mkdir -p. 

Just to be clear: Even if we start using it for MKDIR_P, I still think it's a 
good idea to use our macro. 

/Magnus

> 4 okt. 2018 kl. 00:51 skrev Erik Joelsson :
> 
> Since upgrading our Solaris build machines to 11.3, we have experienced 
> intermittent build failures in the recipes copying java resource files. It's 
> unclear why this started happening now after having worked fine for so long, 
> but it seems it's a race caused by concurrent calls to "mkdir -p". In other 
> recipes we have worked around this by using a macro "MakeDir" which reduces 
> the likelihood of concurrent calls happening. Rewriting these copy and clean 
> rules to use the current preferred macros seem to alleviate the problem for 
> us on Solaris 11.3, and also makes the build a little bit more coherent.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8211677
> 
> Webrev: http://cr.openjdk.java.net/~erikj/8211677/webrev.01/
> 
> /Erik
> 

Re: RFR: 8210459: Add support for generating compile_commands.json

[Magnus Ihse Bursie]

> 4 okt. 2018 kl. 10:33 skrev Robin Westberg :
> 
> Hi Erik,
> 
>> On 3 Oct 2018, at 20:51, Erik Joelsson  wrote:
>> 
>> Hello Robin,
>> 
>> make/CompileCommands.gmk: typo in comment: “prepened"
> 
> Fixed. (Couldn’t resist a slight rewording so the text has reflowed a bit, 
> sorry!)

Much better written, thank you!

> 
>>> On 2018-10-03 11:09, Robin Westberg wrote:
>>> Hi Magnus,
>>> 
>>> Thanks for reviewing, sorry for taking a while to respond!
>>> 
 On 19 Sep 2018, at 13:02, Magnus Ihse Bursie 
  wrote:
 
 19 sep. 2018 kl. 09:51 skrev Magnus Ihse Bursie 
 :
 
> In Awt2dLibraries.gmk, I think lines 733 and 735 needs to be fixed as 
> well. Using FindLib is generally a good idea for this. I suspect 
> there may be more such instances sprinkled around the makefiles.
 Only fixed the last one, I think the first one is ok as is?
>>> The first one is sort of OK, but it's a questionable construct as the 
>>> $(BUILD_LIBAWT_XAWT) variable may contain additional targets. We used 
>>> to do it that way but these days I prefer the more explicit and precise 
>>> FindLib. In this particular case you get a non needed dependency added 
>>> when running compile-commands with ENABLE_HEADLESS_ONLY=true, which 
>>> will not really affect anything so it doesn't really matter.
>> Yeah I certainly agree, but a quick grep shows that there’s about 50 
>> such constructs present right now. I wouldn’t mind cleaning those up, 
>> but perhaps that should be in a separate change?
> If that does not affect your patch, you do not need to clean up those 
> constructs.
> 
> I've now looked through your patch. Overall, it looks good. Some minor 
> comments:
> 
> * In  make/CompileCommands.gmk, are you sure the find -exec + construct 
> does not exceed command line limits on problematic platforms such as 
> Solaris and Windows?
>>> It runs successfully on Windows and Solaris right now at least, and the 
>>> filenames only include a relative path, so should not be sensitive to 
>>> working directory location. But I have to admit I’m not sure how close to 
>>> the limit it is right now.. Perhaps I can build something around “xargs -s” 
>>> instead if you think this is risky?
>> I think it would be good to make sure the file list is split using xargs to 
>> avoid weird failures in the future. I also just realized it would be good to 
>> sort the file list to guarantee stable output.
> 
> Makes sense, changed it to use sort and xargs.

Much better, thanks!

> 
> Webrev (full): http://cr.openjdk.java.net/~rwestberg/8210459/webrev.04/

I was about to say that this looks good, but then I discovered a weird thing. 
I'm the SED line, line 50, there is a weird C-like Unicode character instead of 
a quote, after -e. Looks really odd. Is it a "smart quote" gone wrong? Can you 
look into it and fix it? You don't need to respin the webrev for that. 
Otherwise, you're all good to go. 

/Magnus

> Webrev (incremental): 
> http://cr.openjdk.java.net/~rwestberg/8210459/webrev.03-04/
> 
> Best regards,
> Robin
> 
>> 
>> Otherwise I think this is good now.
>> 
>> /Erik
> The AWT constructs also confuses me. Maybe you can expand a bit on the 
> comment, because it really is non-trivial. You are executing a cp for 
> each tmpfile you find? But what if you find multiple tmpfiles? There 
> could certainly be multiple commands using @ constructs?
>>> What it does it actually to invoke fixpath on everything inside the final 
>>> compile_commands.json file, but in order to make fixpath do that, it 
>>> presents the compile_commands.json file as an @-argument. Fixpath will then 
>>> translate that argument to a temporary filename containing corrected paths, 
>>> and that’s the file we save (since it’s deleted when the invoked command 
>>> terminates). I’ve updated the comment a bit, hopefully it’s more clear now.
>>> 
>>> Another option would be to extend the fixpath utility itself to support 
>>> some additional argument to just do inline path correction on a given file, 
>>> but I felt that it would be a more risky change.
>>> 
> * In make/Main.gmk, you can just do "($(CD) $(OUTPUTDIR) && $(RM) -r 
> build*.log* compile_commands.json)" on a single line.
>>> Fixed.
>>> 
> * In make/common/MakeBase.gmk: I'd prefer if these functions were move to 
> make/common/JdkNativeCompilation.gmk, close to the FindSrcDirsForLib etc 
> definitions.
>>> Fixed.
>>> 
> * In make/common/NativeCompilation.gmk, I'm not entirely happy with the 
> $1_OBJ_COMMON_OPTIONS_PRE/POST construct. I understand what you are 
> trying to achieve, but I think this gets very hard to read. I don't have 
> a perfect solution in mind. But perhaps something like this:
> $1_DEPS_OPTIONS := $$($1_DEP_FLAG) $$($1_DEP).tmp
> $1_COMPILE_COMMAND := $$($1_COMPILER) $$($1_FLAGS) $$($1_DEPS_OPTIONS) 
> 

Re: RFC: JEP JDK-8208089: Implement C++14 Language Features

[Martijn Verburg]

Hi Kim,

I like this initiative.  I'm wondering if some of these rules can be easily
codified or written into a jcheck style checker (ccheck?) so that Authors
can adhere to the conventions and not rely on a Human review to pick out
where that convention isn't met.

Cheers,
Martijn


On Wed, 3 Oct 2018 at 20:13, Kim Barrett  wrote:

> I've submitted a JEP for
>
> (1) enabling the use of C++14 Language Features when building the JDK,
>
> (2) define a process for deciding and documenting which new features
> can be used or are forbidden in HotSpot code,
>
> (3) provide an initial list of permitted and forbidden new features.
>
> https://bugs.openjdk.java.net/browse/JDK-8208089
>
>

Re: RFC: JEP JDK-8208089: Implement C++14 Language Features

[Volker Simonis]

Hi Tim, Jeff,

Kim has assembled a quite detailed list of new C++ features intended for
use in HotSpot/OpenJDK (with links to the corresponding C++ standard)

https://bugs.openjdk.java.net/browse/JDK-8208089

Could you please provide a summary of which of these features are already
available since which version of xlC for AIX and potentially escalate the
implementation of the missing features within IBM’s xlC on AIX team.

Thank you and best regards,
Volker


Kim Barrett  schrieb am Mi. 3. Okt. 2018 um 22:13:

> I've submitted a JEP for
>
> (1) enabling the use of C++14 Language Features when building the JDK,
>
> (2) define a process for deciding and documenting which new features
> can be used or are forbidden in HotSpot code,
>
> (3) provide an initial list of permitted and forbidden new features.
>
> https://bugs.openjdk.java.net/browse/JDK-8208089
>
>

Re: RFR: 8210459: Add support for generating compile_commands.json

[Robin Westberg]

Hi Erik,

> On 3 Oct 2018, at 20:51, Erik Joelsson  wrote:
> 
> Hello Robin,
> 
> make/CompileCommands.gmk: typo in comment: “prepened"

Fixed. (Couldn’t resist a slight rewording so the text has reflowed a bit, 
sorry!)

> On 2018-10-03 11:09, Robin Westberg wrote:
>> Hi Magnus,
>> 
>> Thanks for reviewing, sorry for taking a while to respond!
>> 
>>> On 19 Sep 2018, at 13:02, Magnus Ihse Bursie 
>>>  wrote:
>>> 
>>> 19 sep. 2018 kl. 09:51 skrev Magnus Ihse Bursie 
>>> :
>>> 
 In Awt2dLibraries.gmk, I think lines 733 and 735 needs to be fixed as 
 well. Using FindLib is generally a good idea for this. I suspect there 
 may be more such instances sprinkled around the makefiles.
>>> Only fixed the last one, I think the first one is ok as is?
>> The first one is sort of OK, but it's a questionable construct as the 
>> $(BUILD_LIBAWT_XAWT) variable may contain additional targets. We used to 
>> do it that way but these days I prefer the more explicit and precise 
>> FindLib. In this particular case you get a non needed dependency added 
>> when running compile-commands with ENABLE_HEADLESS_ONLY=true, which will 
>> not really affect anything so it doesn't really matter.
> Yeah I certainly agree, but a quick grep shows that there’s about 50 such 
> constructs present right now. I wouldn’t mind cleaning those up, but 
> perhaps that should be in a separate change?
 If that does not affect your patch, you do not need to clean up those 
 constructs.
 
 I've now looked through your patch. Overall, it looks good. Some minor 
 comments:
 
 * In  make/CompileCommands.gmk, are you sure the find -exec + construct 
 does not exceed command line limits on problematic platforms such as 
 Solaris and Windows?
>> It runs successfully on Windows and Solaris right now at least, and the 
>> filenames only include a relative path, so should not be sensitive to 
>> working directory location. But I have to admit I’m not sure how close to 
>> the limit it is right now.. Perhaps I can build something around “xargs -s” 
>> instead if you think this is risky?
> I think it would be good to make sure the file list is split using xargs to 
> avoid weird failures in the future. I also just realized it would be good to 
> sort the file list to guarantee stable output.

Makes sense, changed it to use sort and xargs.

Webrev (full): http://cr.openjdk.java.net/~rwestberg/8210459/webrev.04/
Webrev (incremental): 
http://cr.openjdk.java.net/~rwestberg/8210459/webrev.03-04/

Best regards,
Robin

> 
> Otherwise I think this is good now.
> 
> /Erik
 The AWT constructs also confuses me. Maybe you can expand a bit on the 
 comment, because it really is non-trivial. You are executing a cp for each 
 tmpfile you find? But what if you find multiple tmpfiles? There could 
 certainly be multiple commands using @ constructs?
>> What it does it actually to invoke fixpath on everything inside the final 
>> compile_commands.json file, but in order to make fixpath do that, it 
>> presents the compile_commands.json file as an @-argument. Fixpath will then 
>> translate that argument to a temporary filename containing corrected paths, 
>> and that’s the file we save (since it’s deleted when the invoked command 
>> terminates). I’ve updated the comment a bit, hopefully it’s more clear now.
>> 
>> Another option would be to extend the fixpath utility itself to support some 
>> additional argument to just do inline path correction on a given file, but I 
>> felt that it would be a more risky change.
>> 
 * In make/Main.gmk, you can just do "($(CD) $(OUTPUTDIR) && $(RM) -r 
 build*.log* compile_commands.json)" on a single line.
>> Fixed.
>> 
 * In make/common/MakeBase.gmk: I'd prefer if these functions were move to 
 make/common/JdkNativeCompilation.gmk, close to the FindSrcDirsForLib etc 
 definitions.
>> Fixed.
>> 
 * In make/common/NativeCompilation.gmk, I'm not entirely happy with the 
 $1_OBJ_COMMON_OPTIONS_PRE/POST construct. I understand what you are trying 
 to achieve, but I think this gets very hard to read. I don't have a 
 perfect solution in mind. But perhaps something like this:
 $1_DEPS_OPTIONS := $$($1_DEP_FLAG) $$($1_DEP).tmp
 $1_COMPILE_COMMAND := $$($1_COMPILER) $$($1_FLAGS) $$($1_DEPS_OPTIONS) 
 $(CC_OUT_OPTION)$$($1_OBJ) $$($1_SRC_FILE))
 
 and for the compile commands, use $$(filter-out $$($1_DEPS_OPTIONS), 
 $$($1_COMPILE_COMMAND)).
 
 Perhaps some unification with the Microsoft toolchain is possible by 
 setting $1_DEPS_OPTIONS to -showIncludes.
 
>>> An alternative, perhaps better, is to move the deps flag to the start. Then 
>>> you could do something like the above, but set
>>> 
>>> $1_COMPILE_OPTIONS := $$($1_FLAGS) $(CC_OUT_OPTION)$$($1_OBJ) 
>>> $$($1_SRC_FILE))
>>> and when compiling do this:
>>> $$($1_COMPILER)  $$($1_DEPS_OPTIONS)  

RE: JEP JDK-8208089: Implement C++14 Language Features

[Lindenmaier, Goetz]

Hi,

I appreciate this is handled in a JEP and well communicated!

XLc, the compiler we use for AIX, might be a bottleneck here.

But we currently have no compiler-constraints by products on 
our aix port of jdk12 (for jdk11 we must stay with the current 
compiler xlc 12 which does not support C++11, and jdk11 even
should be compilable with aCC by HP for which we won't
get new versions!)

We will update our compiler for jdk/jdk to the most recent Xlc
as soon as possible.
To my current knowledge, Xlc 14 was dropped, and xlc 16
is to be released early 2019.  It is supposed to support
C++ 11, and also some C++ 14 features.

Best regards,
  Goetz.



> -Original Message-
> From: core-libs-dev  On Behalf
> Of Kim Barrett
> Sent: Mittwoch, 3. Oktober 2018 21:13
> To: hotspot-dev developers 
> Cc: build-dev ; core-libs-
> d...@openjdk.java.net
> Subject: RFC: JEP JDK-8208089: Implement C++14 Language Features
> 
> I've submitted a JEP for
> 
> (1) enabling the use of C++14 Language Features when building the JDK,
> 
> (2) define a process for deciding and documenting which new features
> can be used or are forbidden in HotSpot code,
> 
> (3) provide an initial list of permitted and forbidden new features.
> 
> https://bugs.openjdk.java.net/browse/JDK-8208089