[PATCH] linux-sparc build fixes

[Erik Helin]

On 06/14/2017 02:04 PM, John Paul Adrian Glaubitz wrote:
> Hi Erik!
>
> On Wed, Jun 14, 2017 at 01:50:06PM +0200, Erik Helin wrote:
>> thanks for contributing and signing the OCA!
>
> Thanks for reviewing my patches ;-).
>
>> I think the first three patches (hotspot-add-missing-log-header.diff,
>> hotspot-fix-checkbytebuffer.diff, rename-sparc-linux-atomic-header.diff) all
>> look good, thanks for fixing broken code. Consider them Reviewed by me.
>> Every patch needs a corresponding issue in the bug tracker, so I went ahead
>> and created:
>> - https://bugs.openjdk.java.net/browse/JDK-8182163
>> - https://bugs.openjdk.java.net/browse/JDK-8182164
>> - https://bugs.openjdk.java.net/browse/JDK-8182165
>
> Great, thank you!
>
>> For the last of those three patches, rename-space-linux-atomic-header.diff,
>> did you do `hg mv` when renaming the file (in order to preserve version
>> control history)?
>
> I'm not 100% whether I did that. I'm not very familar with mercurial
> as I'm more used to git. If the patch format looks wrong to you, I can
> resend a revised version of this patch.

No worries, someone will have to commit your patches anyway (most likely 
me). I can have a look then and ensure that `hg mv` is used for renaming 
the file.

>> For the fourth patch, fix-zero-build-on-sparc.diff, I'm not so sure. For
>> example, the following is a bit surprising to me (mostly because I'm not
>> familiar with zero):
>
> The fourth patch may not be a 100% clean as it's more a result of
> fixing compile errors until the build finished. I can definitely send
> a revised, cleaner version of this patch after more extensive testing.

Yeah, I guessed that was the case :) Without the fourth patch 
(fix-zero-build-on-sparc.diff), does the "regular" linux/sparc build 
compile and run? Is that something you can test?

Also, have you run the tier 1 testing for hotspot (the tests that need 
to pass for every commit)? You can run those tests by running (from the 
top-level "root" repo):

$ make test TEST=hotspot_tier1

or, if you want to try the new run-test functionality

$ make run-test-hotspot_tier1

>> When this code was written, the intent was clearly to have a specialized
>> version of this function for SPARC. When writing such code, do we always
>> have to take into account the zero case with !defined(ZERO)? That doesn't
>> seem like the right (or a scalable) approach to me.
>
> I agree. It's rather suboptimal.

Yes, which is why I want to get a better understanding before I give a 
"thumbs up" for this last patch. I hope (suspect) that there is a better 
way to do this.

>> Severin and/or Roman, do you guys know more about Zero and how this should
>> work? If I want to write a function that I want to specialize for e.g.
>> x86-64 or arm, do I always have to take Zero into account? Or should some
>> other define be used, like #ifdef TARGET_ARCH_sparc?
>
> Thanks a lot for the review!

You are welcome :)

> Can't wait for my first patches getting merged into OpenJDK ;-).

Well, you do need one more reviewer for your patches. Hotspot requires 
at least two reviewers for every patch (and one of the reviewers has to 
have the Reviewer role).

Thanks,
Erik

> Adrian
>