On 27/02/2020 22:09, Simon Tooke wrote: > snip...
>> >> I believe part of the problem here is newer versions of CLang. Could >> some of that testing not be done with CLang on GNU/Linux, so more people >> can participate in getting this fixed? > A backport of 8019470, for example includes macos-specific code; notable > a call to xcode-select, so a test on GNU/Linux would not work. It's > also unclear what internal changes Apple has made to their version of > LLVM, and one of my fixes is definitely compiler-dependent. In > addition, future backports proposed will explicitly disable optimization > of some files because of compiler bugs; this would be very > version-dependent. Sure. I wasn't suggesting it would replace testing on Mac OS. But there's a lot of common ground here that could be covered by getting a Clang build on GNU/Linux working first. >> >> A good starting point seems to be JDK-8019470 [0]. This is actually on >> the Oracle parity list for 8u252, but no-one seems to have attempted the >> backport. > > I actually just tried the backport; it does solve a couple of issues, > but doesn't remove all of the Xcode version checks (so a build won't > happen), and based on prior experience, the JDK that comes out will > crash due to other fixes not being present, fixes that are present in my > patch. > > The Oracle backport, therefore, probably has more changes than in a > simple backport of 8019470, as does my patch. What I would like to do > is incorporate the default toolchain setting if Xcode version > 5. > > If you want, I can submit my backport, then rebase my patch on top of that. > I'm not suggesting JDK-8019470 was a replacement for your webrev. I identified it from looking at your webrev and tracing back the changes. So, yes, that should be backported first rather than swallowed into a fix under a different bug ID. > As for the other content of my webrev, it is pulled together from a > series of partial webrevs in higher-level JDKs, and my own work. > Backporting the entirety of some of those webrevs is infeasible - what > was a one-liner becomes many hundreds of lines, for example in one > case. It is my belief that, although it is desirable to backport most > fixes in their entirety, sometimes the less disruptive option is the > better one. > > (an example: my patch changes about 15 lines, taken from this changeset: > http://hg.openjdk.java.net/jdk/jdk/rev/75aa3e39d02c) > > I appreciate the intent, and in other cases have happily corrected > patches to ensure increased compatiblity with future downport work, but > I suspect in this case, the opposite is true. Sure, but that's something to consider on a case-by-case basis. For the change you refer to, yes, it's pretty obvious it could be simplified, rather than covering four bugs in one changeset. Most of those changes are a result of the JDK-8182299 part of the change, which turns on parentheses. From the bug description, that was deliberate: "Since compiler warnings can detect real bugs, they should be enabled to the extent possible and the source code changed as needed". On the other hand, although that's a lengthy patch, there is little that's onerous or controversial in there. It would need careful review (there's at least one follow-on [0]), but it looks to me like most of it is things like bad types, missing default cases & parentheses, 90%+ of which would benefit other platforms too and no doubt be encountered by compilers there too. Also, Paul Hohensee, the author of this change, is active in 8u and so could perhaps comment on the viability of backporting this. The problem with starting to cherry-pick something like that is you then make the task of anyone backporting that later even harder. Incidentally, note that the changes is from OpenJDK 10, so it actually already has separate JDK and HotSpot changesets [1] [2] Which version of Clang are you testing with? > > Regards, > > -Simon > >> >> I'm not sure of the relevance of rampdown to this. This wouldn't be >> something approved for 8u252 at this stage. 8u-dev will be available >> for commits again as soon as the bug system is switched over & JFR is >> merged. > Yes, you're right. It is irrelevant. >> >> [0] https://bugs.openjdk.java.net/browse/JDK-8019470 >> >> Thanks, > [0] https://bugs.openjdk.java.net/browse/JDK-8184042 [1] http://hg.openjdk.java.net/jdk10/jdk10/jdk/rev/228f0dc4d21a [2] http://hg.openjdk.java.net/jdk10/jdk10/hotspot/rev/c044f8d03932 Thanks, -- Andrew :) Senior Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net) Fingerprint = 5132 579D D154 0ED2 3E04 C5A0 CFDA 0F9B 3596 4222
