Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

8253970: Build error: address argument to atomic builtin must be a pointer to integer or pointer ('volatile narrowOop *' invalid) #496

Closed
wants to merge 8 commits into from
Closed

8253970: Build error: address argument to atomic builtin must be a pointer to integer or pointer ('volatile narrowOop *' invalid) #496

wants to merge 8 commits into from

Conversation

DamonFool
Copy link
Member

@DamonFool DamonFool commented Oct 3, 2020 •
edited by openjdk bot

__sync_val_compare_and_swap shouldn't call with narrowOop* for clang after JDK-8247912.
Before passing type T to __sync_val_compare_and_swap, the fix converts T to uint32_t* if sizeof(T) == 4.

Testing:

  • Zero VM build on Linux and MacOS with clang
  • Zero VM build on Linux with gcc

Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Testing

Linux x64 Windows x64 macOS x64
Build ✔️ (5/5 passed) ✔️ (2/2 passed) ✔️ (2/2 passed)
Test (tier1) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed)

Issue

  • JDK-8253970: Build error: address argument to atomic builtin must be a pointer to integer or pointer ('volatile narrowOop *' invalid)

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/496/head:pull/496
$ git checkout pull/496

@DamonFool
8253970 Build error: address argument to atomic builtin must be a poi…
52ba86d 
…nter to integer or pointer ('volatile narrowOop *' invalid)
@DamonFool
Copy link
Member Author

/issue add JDK-8253970
/test

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 3, 2020

👋 Welcome back jiefu! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label Oct 3, 2020
@openjdk
Copy link

openjdk bot commented Oct 3, 2020

@DamonFool This issue is referenced in the PR title - it will now be updated.

@openjdk
Copy link

openjdk bot commented Oct 3, 2020

@DamonFool The following label will be automatically applied to this pull request:

  • hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Oct 3, 2020
@DamonFool
Copy link
Member Author

/label add hotspot-runtime
/cc hotspot-runtime

@openjdk
Copy link

openjdk bot commented Oct 3, 2020

@DamonFool The hotspot-runtime label was already applied.

@openjdk
Copy link

openjdk bot commented Oct 3, 2020

@DamonFool The hotspot-runtime label was already applied.

@mlbridge
Copy link

mlbridge bot commented Oct 3, 2020 •
edited

Webrevs

@kimbarrett
Copy link

No, don't make this change. Something else is going wrong here. The Platform layer shouldn't be called with enum types; the higher level platform-independent layer should have canonicalized such to the associated underlying integral types.

@kimbarrett
Copy link

Apparently I misremembered. The platform-independent layer isn't doing that canonicalization of enum types to associated integral types. But I think it should be in some places.

@DamonFool
Copy link
Member Author

Thanks @kimbarrett for your review.
I'll think it more after my holiday.

@kimbarrett
Copy link

What version of clang?

gcc (at least recent versions) allows enum types (both scoped and unscoped) for both the __sync_xxx functions and for the __atomic_xxx functions. (The documentation for both say the type can be an integral scalar or pointer type. Enums are not integral types.)

So this seems to be a clang incompatibility with gcc, which may be a clang bug.

The __sync_xxx functions have been legacy since gcc4.7, having been superseded by the __atomic_xxx functions. Could zero be updated here? Would that help? If clang is incompatible with gcc for the __sync_xxx functions, it might also be incompatible for the __atomic_xxx functions.

BTW, the memory ordering by the zero implementation of Atomic::xchg in terms of __sync_lock_test_and_set looks wrong to me. I think the fence() is on the wrong side of the __sync_xxx operation.

@DamonFool
Copy link
Member Author

The following two versions of clang can reproduce the bug.

$ clang -v
Apple clang version 11.0.0 (clang-1100.0.33.16)
Target: x86_64-apple-darwin19.0.0
Thread model: posix

clang -v

clang version 10.0.0-4ubuntu1
Target: x86_64-pc-linux-gnu
Thread model: posix

@DamonFool
Copy link
Member Author

Hi @kimbarrett ,

I replace __sync_val_compare_and_swap whith __atomic_compare_exchange and it really woks.
Thanks for your help.

I also fix the following bug when building zero VM on MacOS.

  • For target hotspot_variant-zero_libjvm_objs_os_bsd_zero.o:
    ./src/hotspot/os_cpu/bsd_zero/os_bsd_zero.cpp:176:21: error: no member named 'in_stack_yellow_reserved_zone' in 'JavaThread'
    if (thread->in_stack_yellow_reserved_zone(addr)) {
    ~~~~~~ ^
    ./src/hotspot/os_cpu/bsd_zero/os_bsd_zero.cpp:177:19: error: no member named 'disable_stack_yellow_reserved_zone' in 'JavaThread'
    thread->disable_stack_yellow_reserved_zone();
    ~~~~~~ ^
    ./src/hotspot/os_cpu/bsd_zero/os_bsd_zero.cpp:180:26: error: no member named 'in_stack_red_zone' in 'JavaThread'
    else if (thread->in_stack_red_zone(addr)) {
    ~~~~~~ ^
    ./src/hotspot/os_cpu/bsd_zero/os_bsd_zero.cpp:181:19: error: no member named 'disable_stack_red_zone' in 'JavaThread'
    thread->disable_stack_red_zone();
    ~~~~~~ ^

Testing:

  • Zero VM builds on Linux/x64 with both clang and gcc
  • Zero VM build on MacOS

As for the memory ordering of the zero implementation, it might be better to file another bug to fix it.
What do you think?

Thanks.
Best regards,
Jie

@DamonFool
Copy link
Member Author

/test

@openjdk
Copy link

openjdk bot commented Oct 13, 2020

@DamonFool you need to get approval to run the tests in tier1 for commits up until 420ead7

@openjdk openjdk bot added the test-request label Oct 13, 2020
@mlbridge
Copy link

mlbridge bot commented Oct 14, 2020

Mailing list message from Kim Barrett on hotspot-runtime-dev:

On Oct 13, 2020, at 9:27 AM, Jie Fu <jiefu at openjdk.java.net> wrote:

On Sun, 4 Oct 2020 11:57:34 GMT, Jie Fu <jiefu at openjdk.org> wrote:

What version of clang?

gcc (at least recent versions) allows enum types (both scoped and unscoped) for both the __sync_xxx functions and for
the __atomic_xxx functions. (The documentation for both say the type can be an integral scalar or pointer type. Enums
are not integral types.) So this seems to be a clang incompatibility with gcc, which may be a clang bug.

The __sync_xxx functions have been legacy since gcc4.7, having been superseded by the __atomic_xxx functions. Could
zero be updated here? Would that help? If clang is incompatible with gcc for the __sync_xxx functions, it might also
be incompatible for the __atomic_xxx functions. BTW, the memory ordering by the zero implementation of Atomic::xchg in
terms of __sync_lock_test_and_set looks wrong to me. I think the fence() is on the wrong side of the __sync_xxx
operation.

The following two versions of clang can reproduce the bug.

$ clang -v
Apple clang version 11.0.0 (clang-1100.0.33.16)
Target: x86_64-apple-darwin19.0.0
Thread model: posix

# clang -v
clang version 10.0.0-4ubuntu1
Target: x86_64-pc-linux-gnu
Thread model: posix

Hi @kimbarrett ,

I replace __sync_val_compare_and_swap whith __atomic_compare_exchange and it really woks.
Thanks for your help.

Good news that __atomic_compare_exchange works where
__sync_val_compare_and_swap doesn't.

However, __ATOMIC_SEQ_CST doesn't provide the same semantics as the __sync
function, nor the required semantics for HotSpot. I think something like
what's in atomic_linux_aarch64.hpp is needed, i.e.

T value = compare_value;
FULL_MEM_BARRIER;
__atomic_compare_exchange(dest, &value, &exchange_value, /*weak/false,
__ATOMIC_RELAXED, __ATOMIC_RELAXED);
FULL_MEM_BARRIER;
return value;

See the discussion related to that linux_aarch64 code here:
https://mail.openjdk.java.net/pipermail/aarch64-port-dev/2019-November/008164.html

I also fix the following bug when building zero VM on MacOS.
--------------------------------------------------------------
* For target hotspot_variant-zero_libjvm_objs_os_bsd_zero.o:
./src/hotspot/os_cpu/bsd_zero/os_bsd_zero.cpp:176:21: error: no member named 'in_stack_yellow_reserved_zone' in
'JavaThread'
if (thread->in_stack_yellow_reserved_zone(addr)) {
~~~~~~ ^
./src/hotspot/os_cpu/bsd_zero/os_bsd_zero.cpp:177:19: error: no member named 'disable_stack_yellow_reserved_zone' in
'JavaThread'
thread->disable_stack_yellow_reserved_zone();
~~~~~~ ^
./src/hotspot/os_cpu/bsd_zero/os_bsd_zero.cpp:180:26: error: no member named 'in_stack_red_zone' in 'JavaThread'
else if (thread->in_stack_red_zone(addr)) {
~~~~~~ ^
./src/hotspot/os_cpu/bsd_zero/os_bsd_zero.cpp:181:19: error: no member named 'disable_stack_red_zone' in 'JavaThread'
thread->disable_stack_red_zone();
~~~~~~ ^

I think this should be a separate bug and PR. This seems to be a result of a recent change
(JDK-8253717, pushed a little less than a week ago) not accounting for bsd_zero; the needed
change seems to have only been made to linux_zero.

As for the memory ordering of the zero implementation, it might be better to file another bug to fix it.
What do you think?

I?m okay with that. The linux_aarch64 implementation looks like a good model for zero.

@mlbridge
Copy link

mlbridge bot commented Oct 14, 2020

Mailing list message from Kim Barrett on hotspot-runtime-dev:

On Oct 13, 2020, at 8:51 PM, Kim Barrett <kim.barrett at oracle.com> wrote:

I also fix the following bug when building zero VM on MacOS.
--------------------------------------------------------------
* For target hotspot_variant-zero_libjvm_objs_os_bsd_zero.o:
./src/hotspot/os_cpu/bsd_zero/os_bsd_zero.cpp:176:21: error: no member named 'in_stack_yellow_reserved_zone' in
'JavaThread'
if (thread->in_stack_yellow_reserved_zone(addr)) {
~~~~~~ ^
./src/hotspot/os_cpu/bsd_zero/os_bsd_zero.cpp:177:19: error: no member named 'disable_stack_yellow_reserved_zone' in
'JavaThread'
thread->disable_stack_yellow_reserved_zone();
~~~~~~ ^
./src/hotspot/os_cpu/bsd_zero/os_bsd_zero.cpp:180:26: error: no member named 'in_stack_red_zone' in 'JavaThread'
else if (thread->in_stack_red_zone(addr)) {
~~~~~~ ^
./src/hotspot/os_cpu/bsd_zero/os_bsd_zero.cpp:181:19: error: no member named 'disable_stack_red_zone' in 'JavaThread'
thread->disable_stack_red_zone();
~~~~~~ ^

I think this should be a separate bug and PR. This seems to be a result of a recent change
(JDK-8253717, pushed a little less than a week ago) not accounting for bsd_zero; the needed
change seems to have only been made to linux_zero.

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

@DamonFool
Copy link
Member Author

Thanks @kimbarrett for your review.
I will spend some time to digest your comments on the memory model semantics.

@DamonFool DamonFool mentioned this pull request Oct 14, 2020
Closed
3 tasks
@DamonFool
Copy link
Member Author

Hi @kimbarrett ,

I agree with you.
All of the atomic operations that imply a read-modify-write action guarantee a two-way memory barrier across that operation.
So FULL_MEM_BARRIERs (before and after the atomic operation) should be added.

The patch has been updated.
Please review it.
Thanks.

Best regards,
Jie

kimbarrett
kimbarrett approved these changes Oct 15, 2020
View reviewed changes
Copy link

@kimbarrett kimbarrett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@openjdk
Copy link

openjdk bot commented Oct 15, 2020 •
edited

@DamonFool This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8253970: Build error: address argument to atomic builtin must be a pointer to integer or pointer ('volatile narrowOop *' invalid)

Reviewed-by: kbarrett, dholmes

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 57 new commits pushed to the master branch:

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Oct 15, 2020
@DamonFool
Copy link
Member Author

Thanks @kimbarrett for your review.
Will push it tomorrow if there is no objection.

@mlbridge
Copy link

mlbridge bot commented Oct 15, 2020

Mailing list message from Kim Barrett on hotspot-runtime-dev:

On Oct 15, 2020, at 2:30 AM, Jie Fu <jiefu at openjdk.java.net> wrote:

On Thu, 15 Oct 2020 03:20:40 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

Jie Fu has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes
the unrelated changes brought in by the merge/rebase. The pull request contains eight additional commits since the last
revision:
- Merge branch 'master' into JDK-8253970
- Add FULL_MEM_BARRIER
- Merge branch 'master' into JDK-8253970
- Revert changes
- Replace __sync_val_compare_and_swap whith __atomic_compare_exchange
- Merge branch 'master' into JDK-8253970
- Revert changes
- 8253970 Build error: address argument to atomic builtin must be a pointer to integer or pointer ('volatile narrowOop *'
invalid)

Looks good.

Thanks @kimbarrett for your review.
Will push it tomorrow if there is no objection.

There should normally be a second reviewer for HotSpot changes.

@DamonFool
Copy link
Member Author

OK.

May I get a second review for this change?
Thanks.

@DamonFool
Copy link
Member Author

Hi all,

May I get a second review to finish this issue?
Some of our testing pipelines become red due to this bug.

Thanks.
Best regards,
Jie

dholmes-ora
dholmes-ora approved these changes Oct 19, 2020
View reviewed changes
Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems consistent with the aarch64 code.

Thanks,
David

@DamonFool
Copy link
Member Author

Thanks @dholmes-ora for your review.
/integrate

@openjdk openjdk bot closed this Oct 19, 2020
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 19, 2020
@openjdk
Copy link

openjdk bot commented Oct 19, 2020

@DamonFool Since your change was applied there have been 57 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

Pushed as commit cb7701b.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@DamonFool DamonFool deleted the JDK-8253970 branch October 19, 2020 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dholmes-ora dholmes-ora dholmes-ora approved these changes

@kimbarrett kimbarrett kimbarrett approved these changes

Assignees
No one assigned
Labels
hotspot-runtime hotspot-runtime-dev@openjdk.org integrated Pull request has been integrated test-request
Milestone
No milestone
3 participants
@DamonFool @kimbarrett @dholmes-ora