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

8320533: Adjust capstone integration for v6 changes #16788

Closed
wants to merge 4 commits into from

Conversation

galderz
Copy link
Contributor

@galderz galderz commented Nov 23, 2023

FYI @theRealAph

It includes a couple of commits to integrate with Capstone v6 while still working with Capstone v5 and before:

  • CAPSTONE_ARCH for aarch64 is now CS_ARCH_AARCH64 instead of CS_ARCH_ARM64. See this document to understand motivation for this Capstone change.
  • The -Daarch64 macro was getting in the way and was causing invalid C to be produced. Undefined it before including capstone.h. Thx @rwestrel for suggesting the fix!
  • Enhanced autoconf to select the right aarch64 arch name depending on the capstone library in use.

Here's some output to demonstrate autoconf:

  1. If using capstone v6, you will see:
checking capstone aarch64 arch name... AARCH64
  1. If using capstone v5, or earlier, you will see:
checking capstone aarch64 arch name... ARM64

With v5 or earlier, the compilation error in the config log file is the expected one:

configure:142906: checking capstone aarch64 arch name
configure:142919: /usr/bin/clang -c  -arch arm64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.0.sdk -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.0.sdk/System/Library/Frameworks  -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.0.sdk -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.0.sdk/System/Library/Frameworks conftest.c >&5
conftest.c:27:16: error: use of undeclared identifier 'CS_ARCH_AARCH64'; did you mean 'CS_ARCH_ARM64'?
cs_arch test = CS_ARCH_AARCH64
               ^~~~~~~~~~~~~~~
               CS_ARCH_ARM64
/Users/galder/opt/capstone-5/include/capstone/capstone.h:76:2: note: 'CS_ARCH_ARM64' declared here
        CS_ARCH_ARM64,          ///< ARM-64, also called AArch64
        ^
1 error generated.
configure:142919: $? = 1
configure: failed program was:
| /* confdefs.h */
| #define PACKAGE_NAME "OpenJDK"
| #define PACKAGE_TARNAME "openjdk"
| #define PACKAGE_VERSION "openjdk"
| #define PACKAGE_STRING "OpenJDK openjdk"
| #define PACKAGE_BUGREPORT "build-dev@openjdk.org"
| #define PACKAGE_URL "https://openjdk.org"
| #define HAVE_STDIO_H 1
| #define HAVE_STDLIB_H 1
| #define HAVE_STRING_H 1
| #define HAVE_INTTYPES_H 1
| #define HAVE_STDINT_H 1
| #define HAVE_STRINGS_H 1
| #define HAVE_SYS_STAT_H 1
| #define HAVE_SYS_TYPES_H 1
| #define HAVE_UNISTD_H 1
| #define STDC_HEADERS 1
| #define HAVE_STDIO_H 1
| #define SIZEOF_INT_P 8
| #define HAVE_CUPS_CUPS_H 1
| #define HAVE_CUPS_PPD_H 1
| /* end confdefs.h.  */
| #include "/Users/galder/opt/capstone-5/include/capstone/capstone.h"
| int
| main (void)
| {
| cs_arch test = CS_ARCH_AARCH64
|   ;
|   return 0;
| }

Happy to squash both commits together if it makes sense.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8320533: Adjust capstone integration for v6 changes (Bug - P4)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/16788/head:pull/16788
$ git checkout pull/16788

Update a local copy of the PR:
$ git checkout pull/16788
$ git pull https://git.openjdk.org/jdk.git pull/16788/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16788

View PR using the GUI difftool:
$ git pr show -t 16788

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/16788.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 23, 2023

👋 Welcome back galderz! 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 Nov 23, 2023
@openjdk
Copy link

openjdk bot commented Nov 23, 2023

@galderz The following labels will be automatically applied to this pull request:

  • build
  • hotspot-compiler

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

@openjdk openjdk bot added build build-dev@openjdk.org hotspot-compiler hotspot-compiler-dev@openjdk.org labels Nov 23, 2023
@mlbridge
Copy link

mlbridge bot commented Nov 23, 2023

Webrevs

@openjdk
Copy link

openjdk bot commented Nov 23, 2023

@galderz Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

@@ -52,6 +52,7 @@
#include <inttypes.h>
#include <string.h>

Copy link
Contributor

Choose a reason for hiding this comment

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

Need a comment about this undef

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment.

Copy link
Member

Choose a reason for hiding this comment

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

Please also update the copyright year for this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

Copy link
Member

@magicus magicus left a comment

Choose a reason for hiding this comment

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

Build changes look fine

@galderz
Copy link
Contributor Author

galderz commented Nov 24, 2023

/integrate

@openjdk
Copy link

openjdk bot commented Nov 24, 2023

@galderz This pull request has not yet been marked as ready for integration.

@theRealAph
Copy link
Contributor

/sponsor

@openjdk
Copy link

openjdk bot commented Nov 24, 2023

@theRealAph The change author (@galderz) must issue an integrate command before the integration can be sponsored.

@magicus
Copy link
Member

magicus commented Nov 24, 2023

@galderz "Integration blocker ⚠️ Title mismatch between PR and JBS for issue JDK-8320533"

* `CAPSTONE_ARCH` for aarch64 in v6 is `CS_ARCH_AARCH64` instead
of `CS_ARCH_ARM64`.
* The `-Daarch64` macro is getting in the way with this new version,
and it's causing invalid C to be produced.
Undefined it before including `capstone.h`.
@galderz
Copy link
Contributor Author

galderz commented Nov 27, 2023

/integrate

@openjdk
Copy link

openjdk bot commented Nov 27, 2023

@galderz Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

@openjdk
Copy link

openjdk bot commented Nov 27, 2023

@galderz This pull request has not yet been marked as ready for integration.

@galderz
Copy link
Contributor Author

galderz commented Nov 27, 2023

Hmmm, had to change the title.

@magicus
Copy link
Member

magicus commented Nov 27, 2023

The PR title must be the same (after the bug ID and colon) as the JBS title. I updated the title in JBS for you (since the PR title seemed better to describe the changes imho). When the bots detect this (will happen shortly), they will remove the integration blocker, and you will be able to issue /integrate.

@openjdk
Copy link

openjdk bot commented Nov 27, 2023

⚠️ @galderz the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout topic.2211.capstone-next
$ git commit --author='Preferred Full Name <you@example.com>' --allow-empty -m 'Update full name'
$ git push

@openjdk
Copy link

openjdk bot commented Nov 27, 2023

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

8320533: Adjust capstone integration for v6 changes

Reviewed-by: ihse, aph

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

  • 5e24aaf: 8320001: javac crashes while adding type annotations to the return type of a constructor
  • f9e9131: 8319703: Serial: Remove generationSpec
  • a006d7e: 8294549: configure script should detect unsupported path
  • 4977922: 8320330: Improve implementation of RShift Value
  • a40d8d9: 8314220: Configurable InlineCacheBuffer size
  • 1272368: 8318113: CSS.BackgroundImage doesn't implement equals
  • 28d3762: 8320618: NPE: Cannot invoke "java.lang.constant.ClassDesc.isArray()" because "this.sym" is null
  • f6e5559: 8320601: ProblemList java/lang/invoke/lambda/LambdaFileEncodingSerialization.java on linux-all
  • bddcd08: 8304701: Request with timeout aborts later in-flight request on HTTP/1.1 cxn
  • 91279fc: 8319778: Remove unreachable code in ObjectSynchronizer::exit
  • ... and 35 more: https://git.openjdk.org/jdk/compare/8b47a14958913c70291d46afdde4e527f9bdc91a...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@magicus, @theRealAph) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Nov 27, 2023
@galderz
Copy link
Contributor Author

galderz commented Nov 27, 2023

⚠️ @galderz the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout topic.2211.capstone-next
$ git commit --author='Preferred Full Name <you@example.com>' --allow-empty -m 'Update full name'
$ git push

What's the issue with this? Full name on my profile "Galder Zamarreño". Head commit output:

commit 9136b1ecdd6deba1ae1902405109bb61068da810 (HEAD -> topic.2211.capstone-next, origin/topic.2211.capstone-next)
Author: Galder Zamarreño <galder@openjdk.org>
Date:   Mon Nov 27 08:34:26 2023 +0100

    8320533: Updated copyright year

@magicus
Copy link
Member

magicus commented Nov 27, 2023

What's the issue with this? Full name on my profile "Galder Zamarreño".

I don't know really. Could be a bug with the Skara bots that choke on the ñ (but I think we've had users with accented characters in their username before, so it should not be an issue).

This is just a warning, not an integration blocker. If you think the HEAD author name looks fine, then you don't need to do anything. If you see this repeatedly (which might indicate a bug in Skara), you can report this on skara-dev@openjdk.org.

@galderz
Copy link
Contributor Author

galderz commented Nov 27, 2023

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Nov 27, 2023
@openjdk
Copy link

openjdk bot commented Nov 27, 2023

@galderz
Your change (at version 9136b1e) is now ready to be sponsored by a Committer.

@erikj79
Copy link
Member

erikj79 commented Nov 27, 2023

What's the issue with this? Full name on my profile "Galder Zamarreño".

I've dug into this a bit and here is what I've found. The letter ñ seems to have been encoded in two different ways in the GitHub user object and in the commit. Internally in the Skara bot, we get the GitHub user full name field returned as an 8 bit String where the characters is encoded as 0xF1. In the commit data from GitHub, the author field is encoded as a 16 bit String where the character is encoded as the composite of 006E and 0303. When we do a String.equals on these strings, it returns false. I can't say if that is correct but that is what's happening. If the 16 bit string was 00F1 it would have been fine.

Since you have an OpenJDK user, I would recommend that you associate your GitHub account with your OpenJDK username. This will avoid this check in the future.

@galderz
Copy link
Contributor Author

galderz commented Nov 28, 2023

Thanks a lot @erikj79 for looking into it. I've created the task to associate my username.

@adinn
Copy link
Contributor

adinn commented Nov 28, 2023

/sponsor

@openjdk
Copy link

openjdk bot commented Nov 28, 2023

Going to push as commit 20aae3c.
Since your change was applied there have been 55 commits pushed to the master branch:

  • 0678253: 8320602: Lock contention in SchemaDVFactory.getInstance()
  • f1a24f6: 8318599: HttpURLConnection cache issues leading to crashes in JGSS w/ native GSS introduced by 8303809
  • 7848ed7: 8301856: Generated .spec file for RPM installers uninstalls desktop launcher on update
  • 726f854: 8320706: RuntimePackageTest.testUsrInstallDir test fails on Linux
  • 1bb250c: 8261837: SIGSEGV in ciVirtualCallTypeData::translate_from
  • 5f7f2c4: 8320249: tools/jpackage/share/AddLauncherTest.java#id1 fails intermittently on Windows in verifyDescription
  • 6871a2f: 8320803: Update SourceVersion.RELEASE_22 description for language changes
  • 82967f4: 8310159: Bulk copy with Unsafe::arrayCopy is slower compared to memcpy
  • f0a12c5: 8320763: Fix spacing arround assignment in spec.gmk.in
  • 12e983a: 8194743: Compiler implementation for Statements before super()
  • ... and 45 more: https://git.openjdk.org/jdk/compare/8b47a14958913c70291d46afdde4e527f9bdc91a...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Nov 28, 2023
@openjdk openjdk bot closed this Nov 28, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Nov 28, 2023
@openjdk
Copy link

openjdk bot commented Nov 28, 2023

@adinn @galderz Pushed as commit 20aae3c.

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

@erikj79
Copy link
Member

erikj79 commented Nov 29, 2023

What's the issue with this? Full name on my profile "Galder Zamarreño".

I've dug into this a bit and here is what I've found. The letter ñ seems to have been encoded in two different ways in the GitHub user object and in the commit. Internally in the Skara bot, we get the GitHub user full name field returned as an 8 bit String where the characters is encoded as 0xF1. In the commit data from GitHub, the author field is encoded as a 16 bit String where the character is encoded as the composite of 006E and 0303. When we do a String.equals on these strings, it returns false. I can't say if that is correct but that is what's happening. If the 16 bit string was 00F1 it would have been fine.

Since you have an OpenJDK user, I would recommend that you associate your GitHub account with your OpenJDK username. This will avoid this check in the future.

I've filed https://bugs.openjdk.org/browse/SKARA-2111 to better deal with unicode string comparisons. We should normalize the strings before comparing.

@galderz galderz deleted the topic.2211.capstone-next branch December 7, 2023 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build build-dev@openjdk.org hotspot-compiler hotspot-compiler-dev@openjdk.org integrated Pull request has been integrated
6 participants