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

8299260: libawt and libfreetype should export only explicitly requested symbols #11742

Closed
wants to merge 1 commit into from

Conversation

djelinski
Copy link
Member

@djelinski djelinski commented Dec 20, 2022

Please review this patch that removes unnecessary exports from libawt and libfreetype.

Verified that:

  • mach5 client libs tests pass
  • both release and debug builds finish successfully

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-8299260: libawt and libfreetype should export only explicitly requested symbols

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11742

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Dec 20, 2022

👋 Welcome back djelinski! 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 changed the title 8200192 8200192: Verify exported symbols in java.desktop Dec 20, 2022
@openjdk
Copy link

openjdk bot commented Dec 20, 2022

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

  • build
  • client

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 client client-libs-dev@openjdk.org labels Dec 20, 2022
@djelinski djelinski marked this pull request as ready for review December 20, 2022 19:58
@openjdk openjdk bot added the rfr Pull request is ready for review label Dec 20, 2022
@mlbridge
Copy link

mlbridge bot commented Dec 20, 2022

Webrevs

@prrace
Copy link
Contributor

prrace commented Dec 20, 2022

Hmm, not easy to know for sure what might be affected. But it's early in JDK 21, so ...
Since you made a change to the freetype build, did you build with --with-freetype=bundled ?
If not, then the build on Linux defaults to using the system freetype and does not use the one in the sources.

@djelinski
Copy link
Member Author

yes, I did build --with-freetype=bundled, and I compared the results of readelf -s between the system libfreetype and the one produced by the build.

After these changes, all of the bundled exports were present in system library. The following exports of the system library were absent in the bundled one:

FTC_CMapCache_Lookup
FTC_CMapCache_New
FTC_ImageCache_L[...]
FTC_ImageCache_Lookup
FTC_ImageCache_New
FTC_Manager_Done
FTC_Manager_Look[...]
FTC_Manager_Look[...]
FTC_Manager_New
FTC_Manager_Remo[...]
FTC_Manager_Reset
FTC_Node_Unref
FTC_SBitCache_Lo[...]
FTC_SBitCache_Lookup
FTC_SBitCache_New
FT_ClassicKern_Free
FT_ClassicKern_V[...]
FT_Error_String
FT_Get_BDF_Charset_ID
FT_Get_BDF_Property
FT_Get_PFR_Advance
FT_Get_PFR_Kerning
FT_Get_PFR_Metrics
FT_Get_WinFNT_Header
FT_Gzip_Uncompress
FT_OpenType_Free
FT_OpenType_Validate
FT_Palette_Data_Get
FT_Palette_Select
FT_Palette_Set_F[...]
FT_Stream_OpenBzip2
FT_Stream_OpenGzip
FT_Stream_OpenLZW
FT_TrueTypeGX_Free
FT_TrueTypeGX_Va[...]

These exports were absent even before my changes, so I believe we should be fine here.

@prrace
Copy link
Contributor

prrace commented Dec 20, 2022

These exports were absent even before my changes, so I believe we should be fine here.

I can't speak to all of these but we only have a subset of freetype in JDK sources, since for example we have no interest in the bitmap formats, so some of the differences are no surprise.

I don't have anything definite to say will be broken by this, so I'll approve this so we can get it in to the builds and we'll see if it causes problems.

@openjdk
Copy link

openjdk bot commented Dec 20, 2022

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

8299260: libawt and libfreetype should export only explicitly requested symbols

Reviewed-by: prr, aivanov, serb

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

  • 2294f22: 8286311: remove boilerplate from use of runTests
  • 5e2de89: 8299194: CustomTzIDCheckDST.java may fail at future date
  • 6ccee83: 8292206: TestCgroupMetrics.java fails as getMemoryUsage() is lower than expected
  • b378381: 8299199: Avoid redundant split calls in FontConfiguration.initReorderMap implementations
  • 62a033e: 8299191: Unnecessarily global friend functions for relocInfo
  • a3693cc: 8295087: Manual Test to Automated Test Conversion
  • 5012039: 8298887: On the latest macOS+XCode the Robot API may report wrong colors
  • 34cdda5: Merge
  • 22007a1: 8298893: Rename option UsePolyIntrinsics to UsePoly1305Intrinsics
  • 9adc349: 8298726: (fs) Change PollingWatchService to record last modified time as FileTime rather than milliseconds
  • ... and 42 more: https://git.openjdk.org/jdk/compare/36de61c460d7038019294293143e420dfcce2936...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 20, 2022
@aivanov-jdk
Copy link
Member

Did not verify the other issues reported in the linked JBS ticket.

Then I suggest filing a new bug to track additional issues raised in JDK-8200192 and editing the description and the subject of JDK-8200192 correspondingly to reflect the current fix only.

@djelinski
Copy link
Member Author

Thanks for the suggestion! I'll create a new bug for this fix instead, and leave a comment on the existing one.

@djelinski djelinski changed the title 8200192: Verify exported symbols in java.desktop 8299260: Libawt should only export explicitly requested symbols Dec 22, 2022
@aivanov-jdk
Copy link
Member

aivanov-jdk commented Dec 22, 2022

Thanks for the suggestion! I'll create a new bug for this fix instead, and leave a comment on the existing one.

Thank you! This way it's clearer.

What about libfreetype? You also modify its settings.

libawt and libfreetype should export only explicitly requested symbols

Does this subject look good?

Copy link
Member

@aivanov-jdk aivanov-jdk left a comment

Choose a reason for hiding this comment

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

The number of exported symbols from libawt and libfreetype is much lower now: 192 vs 970 and 222 vs 560.

I ran clientlibs tests, no failures detected.

@djelinski djelinski changed the title 8299260: Libawt should only export explicitly requested symbols 8299260: libawt and libfreetype should export only explicitly requested symbols Dec 22, 2022
@djelinski
Copy link
Member Author

/integrate

@openjdk
Copy link

openjdk bot commented Dec 30, 2022

Going to push as commit 684e506.
Since your change was applied there have been 70 commits pushed to the master branch:

  • 6f85a9c: 8299387: CompressedClassPointers.java still fails on ppc with 'Narrow klass shift: 0' missing
  • d490f15: 8235297: sun/security/ssl/SSLSessionImpl/ResumptionUpdateBoundValues.java fails intermittent
  • 11fd651: 8298875: A module requiring "java.base" with flags ACC_SYNTHETIC should be rejected
  • 0459159: 8288204: GVN Crash: assert() failed: correct memory chain
  • 19373b2: Merge
  • 188911c: 8299241: jdk/jfr/api/consumer/streaming/TestJVMCrash.java generates unnecessary core file
  • 26868c1: 8299255: Unexpected round errors in FreetypeFontScaler
  • 5e861e3: 8298645: JNI works with accessibleSelection on a wrong thread
  • fd746a2: 8298643: JNI call of getAccessibleRowWithIndex and getAccessibleColumnWithIndex on a wrong thread
  • da75de3: 8299172: RISC-V: [TESTBUG] Fix stack alignment logic in jvmci RISCV64TestAssembler.java
  • ... and 60 more: https://git.openjdk.org/jdk/compare/36de61c460d7038019294293143e420dfcce2936...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Dec 30, 2022
@openjdk openjdk bot closed this Dec 30, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 30, 2022
@openjdk
Copy link

openjdk bot commented Dec 30, 2022

@djelinski Pushed as commit 684e506.

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

@djelinski djelinski deleted the awt-export-cleanup branch March 8, 2023 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build build-dev@openjdk.org client client-libs-dev@openjdk.org integrated Pull request has been integrated
Development

Successfully merging this pull request may close these issues.

4 participants