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

8257467: [TESTBUG] -Wdeprecated-declarations is reported at sigset() in exesigtest.c #1529

Closed
wants to merge 4 commits into from

Conversation

YaSuenag
Copy link
Member

@YaSuenag YaSuenag commented Dec 1, 2020

sigset() is deprecated, and __attribute_deprecated_msg__ has been set to the declaration in glibc

We can see the warning on make test-image as below:

  • Fedora 33 x86_64
    • gcc: gcc-10.2.1-6.fc33.x86_64
    • glibc: glibc-2.32-2.fc33.x86_64
/home/ysuenaga/github-forked/jdk/test/hotspot/jtreg/runtime/signal/exesigtest.c: In function 'setSignalHandler':
/home/ysuenaga/github-forked/jdk/test/hotspot/jtreg/runtime/signal/exesigtest.c:245:9: error: 'sigset' is deprecated: Use the signal and sigprocmask functions instead [-Werror=deprecated-declarations]
  245 | sigset(signal_num, handler);
      | ^~~~~~
In file included from /home/ysuenaga/github-forked/jdk/test/hotspot/jtreg/runtime/signal/exesigtest.c:25:
/usr/include/signal.h:353:23: note: declared here
  353 | extern __sighandler_t sigset (int __sig, __sighandler_t __disp) __THROW
      | ^~~~~~
cc1: all warnings being treated as errors
gmake[3]: *** [test/JtregNativeHotspot.gmk:1525: /home/ysuenaga/github-forked/jdk/build/linux-x86_64-server-fastdebug/support/test/hotspot/jtreg/native/support/exesigtest/exesigtest.o] Error 1
gmake[3]: *** Waiting for unfinished jobs....
gmake[2]: *** [make/Main.gmk:612: build-test-hotspot-jtreg-native] Error 2

Progress

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

Issue

  • JDK-8257467: [TESTBUG] -Wdeprecated-declarations is reported at sigset() in exesigtest.c

Reviewers

Download

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

@bridgekeeper
Copy link

bridgekeeper bot commented Dec 1, 2020

👋 Welcome back ysuenaga! 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
Copy link

openjdk bot commented Dec 1, 2020

@YaSuenag 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 Dec 1, 2020
@YaSuenag YaSuenag marked this pull request as ready for review Dec 1, 2020
@openjdk openjdk bot added the rfr Pull request is ready for review label Dec 1, 2020
@mlbridge
Copy link

mlbridge bot commented Dec 1, 2020

Webrevs

Copy link
Member

@dholmes-ora dholmes-ora left a comment

I would suggest we just delete the sigset usage as it is obsolete. The whole point of this test is to test signal chaining for the VM and sigset is undefined for a multi-threaded process (POSIX spec.) The use of sigaction suffices IMO.
Thanks,
David

@YaSuenag
Copy link
Member Author

YaSuenag commented Dec 1, 2020

It's better if we can remove sigset() from signal handler tests. So I did that in new commit. Could you review again?
This commit passed hotspot/jtreg/runtime/signal tests.

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Just eradicate "mode" completely.

Thanks,
David

test/hotspot/jtreg/runtime/signal/README Outdated Show resolved Hide resolved
test/hotspot/jtreg/runtime/signal/SigTestDriver.java Outdated Show resolved Hide resolved
test/hotspot/jtreg/runtime/signal/exesigtest.c Outdated Show resolved Hide resolved
@YaSuenag
Copy link
Member Author

YaSuenag commented Dec 1, 2020

@dholmes-ora Thanks for your comment! I fixed them in new commit. Could you check again?
This commit passed hotspot/jtreg/runtime/signal tests.

@YaSuenag
Copy link
Member Author

YaSuenag commented Dec 1, 2020

Linux x64 tier1 test (runtime/handshake/HandshakeWalkStackTest.java) failed, but it does not seem to cause by this change.

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Looks good to me!

Perhaps @tstuefe could also take a look?

Thanks,
David

@openjdk
Copy link

openjdk bot commented Dec 2, 2020

@YaSuenag 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:

8257467: [TESTBUG] -Wdeprecated-declarations is reported at sigset() in exesigtest.c

Reviewed-by: dholmes, stuefe

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 34 new commits pushed to the master branch:

  • 282cb32: 8005970: Mouse cursor is default cursor over TextArea's scrollbar
  • f2a0988: 8257228: G1: SIGFPE in G1ConcurrentRefine::create(int*) due to buffers_to_cards overflow
  • fe5cccc: 8254631: Better support ALPN byte wire values in SunJSSE
  • 541c7f7: 8257434: jpackage fails to create rpm on Fedora Linux
  • 8f4fa3f: 8257232: CompileThresholdScaling fails to work on 32-bit platforms
  • cfd070e: 8257537: [vector] Cleanup redundant bitwise cases on floating point vectors
  • 03f3b8e: 8210253: Clipped UI rendering with X11 pipeline and HiDPI
  • ce496cb: 8257190: simplify PhaseIdealLoop constructors
  • 927504e: 8256474: Migrate Mutex _owner accesses to use Atomic operations
  • 00e79db: 8257511: JDK-8254082 brings regression to AbstractStringBuilder.insert(int dstOffset, CharSequence s, int start, int end)
  • ... and 24 more: https://git.openjdk.java.net/jdk/compare/29f86e00b1152c47c77cc8166ce3b9eeb6292804...master

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 Dec 2, 2020
@tstuefe
Copy link
Member

tstuefe commented Dec 2, 2020

Hi,

sorry for chiming in too late, I did not see this before.

I would not remove the sigset testing from this test unless we also remove the sigset chaining capability from libjsig. Otherwise we just ship untested functionality.

So I'd say either go the full way and remove sigset() chaining from src/java.base/unix/native/libjsig/jsig.c. But that probably needs a CSR as well. Or we just go with Yasumasa's initial patch, just switching off the warning.

Cheers, Thomas

@dholmes-ora
Copy link
Member

dholmes-ora commented Dec 2, 2020

Hi Thomas (@tstuefe ),
Thanks for chiming in. I was ready to agree with you but something was nagging at me. Sure enough looking closer signal chaining supports three modes: sigset, sigaction and signal. But the test only tests sigset and sigaction - so we already have untested functionality. And that was because signal() use was broken and we removed it under:

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

back in 2016.

So we do have a precedence (perhaps a bad one) for removing test code without removing functional code.

But I'm okay with going back to Yasumasa's original proposal to just fix the warning and filing a RFE to deprecate, then remove sigset and signal from the signal chaining mechanism.

Thanks,
David

@YaSuenag
Copy link
Member Author

YaSuenag commented Dec 2, 2020

But I'm okay with going back to Yasumasa's original proposal to just fix the warning

Reverted commits for removing sigset tests.

and filing a RFE to deprecate, then remove sigset and signal from the signal chaining mechanism.

I can file it, but I wonder how we mark deprecate to sigset and signal? #pragma in each compilers?
Or can we remove them immediately after CSR is approved?

@tstuefe
Copy link
Member

tstuefe commented Dec 2, 2020

But I'm okay with going back to Yasumasa's original proposal to just fix the warning

Reverted commits for removing sigset tests.

and filing a RFE to deprecate, then remove sigset and signal from the signal chaining mechanism.

I can file it, but I wonder how we mark deprecate to sigset and signal? #pragma in each compilers?
Or can we remove them immediately after CSR is approved?

I think the official Oracle doc for signal chaining would have to be adapted (https://docs.oracle.com/javase/9/vm/signal-chaining.htm).

tstuefe
tstuefe approved these changes Dec 2, 2020
Copy link
Member

@tstuefe tstuefe left a comment

LGTM

@YaSuenag
Copy link
Member Author

YaSuenag commented Dec 2, 2020

and filing a RFE to deprecate, then remove sigset and signal from the signal chaining mechanism.

I can file it, but I wonder how we mark deprecate to sigset and signal? #pragma in each compilers?
Or can we remove them immediately after CSR is approved?

I think the official Oracle doc for signal chaining would have to be adapted (https://docs.oracle.com/javase/9/vm/signal-chaining.htm).

Thanks @tstuefe !
Anyway, I will integrate this change.

@mlbridge
Copy link

mlbridge bot commented Dec 2, 2020

Mailing list message from David Holmes on hotspot-runtime-dev:

On 2/12/2020 6:31 pm, Yasumasa Suenaga wrote:

On Wed, 2 Dec 2020 08:04:27 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

Yasumasa Suenaga has updated the pull request incrementally with one additional commit since the last revision:

Revert "Remove sigset() from signal handler tests"

This reverts commit 21437c3.

LGTM

and filing a RFE to deprecate, then remove sigset and signal from the signal chaining mechanism.

I can file it, but I wonder how we mark deprecate to sigset and signal? `#pragma` in each compilers?
Or can we remove them immediately after CSR is approved?

I think the official Oracle doc for signal chaining would have to be adapted (https://docs.oracle.com/javase/9/vm/signal-chaining.htm).

Thanks @tstuefe !
Anyway, I will integrate this change.

I've filed JDK-8257572 to update signal chaining too deprecate sigset
and signal usage.

David

@YaSuenag
Copy link
Member Author

YaSuenag commented Dec 2, 2020

/integrate

@openjdk openjdk bot closed this Dec 2, 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 Dec 2, 2020
@openjdk
Copy link

openjdk bot commented Dec 2, 2020

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

  • 9de283b: 8257505: nsk/share/test/StressOptions stressTime is scaled in getter but not when printed
  • 282cb32: 8005970: Mouse cursor is default cursor over TextArea's scrollbar
  • f2a0988: 8257228: G1: SIGFPE in G1ConcurrentRefine::create(int*) due to buffers_to_cards overflow
  • fe5cccc: 8254631: Better support ALPN byte wire values in SunJSSE
  • 541c7f7: 8257434: jpackage fails to create rpm on Fedora Linux
  • 8f4fa3f: 8257232: CompileThresholdScaling fails to work on 32-bit platforms
  • cfd070e: 8257537: [vector] Cleanup redundant bitwise cases on floating point vectors
  • 03f3b8e: 8210253: Clipped UI rendering with X11 pipeline and HiDPI
  • ce496cb: 8257190: simplify PhaseIdealLoop constructors
  • 927504e: 8256474: Migrate Mutex _owner accesses to use Atomic operations
  • ... and 25 more: https://git.openjdk.java.net/jdk/compare/29f86e00b1152c47c77cc8166ce3b9eeb6292804...master

Your commit was automatically rebased without conflicts.

Pushed as commit fb139cf.

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

@YaSuenag YaSuenag deleted the JDK-8257467 branch Dec 18, 2020
@phohensee
Copy link
Member

phohensee commented Nov 30, 2021

/backport jdk11u-dev

@openjdk
Copy link

openjdk bot commented Nov 30, 2021

@phohensee Unknown command backport - for a list of valid commands use /help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-runtime hotspot-runtime-dev@openjdk.org integrated Pull request has been integrated
4 participants