8328074: Add jcheck whitespace checking for assembly files #18268

magicus · 2024-03-13T11:18:20Z

As part of the ongoing effort to enable jcheck whitespace checking to all text files, it is now time to address assembly (.S) files. The hotspot assembly files were fixed as part of the hotspot mapfile removal, so only a single incorrect jdk library remains.

The main issue with libjsvml was inconsistent line starts, sometimes using tabs. I used the expand unix command line tool to replace these with spaces.

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-8328074: Add jcheck whitespace checking for assembly files (Enhancement - P4)

Reviewers

Erik Joelsson (@erikj79 - Reviewer)
Sandhya Viswanathan (@sviswa7 - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18268

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-03-13T11:18:49Z

👋 Welcome back ihse! 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 · 2024-03-13T11:19:51Z

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

build

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.

mlbridge · 2024-03-13T11:22:17Z

Webrevs

03: [Full](Webrev is not available because diff is too large) - Incremental (4e729cb6)
02: [Full](Webrev is not available because diff is too large) - Incremental (ce1d4c9f)
01: [Full](Webrev is not available because diff is too large) - Incremental (49fb1016)
00: [Full](Webrev is not available because diff is too large) (ce1d4c9f)

TheShermanTanker · 2024-03-13T11:26:14Z

src/jdk.incubator.vector/linux/native/libjsvml/jsvml_d_acos_linux_x86.S

 ..TXTST0:
 .L_2__routine_start___jsvml_acos2_ha_l9_0:
 # -- Begin  __jsvml_acos2_ha_l9
-	.text
+        .text
 # mark_begin;
       .align    16,0x90


.align seems to ironically not be aligned with the rest of the directives

Bad indentation like this is not something jcheck can detect or will complain about. However, I agree that it looks horrible. What's more, this seem to be problematic in multiple locations -- many, but not all, instances of .align (and ALIGN on Windows) are unaligned. I guess it is due to copy/paste error.

I'm sort of reluctant to fix this in this PR, but then again, it just looks too bad.

magicus · 2024-03-13T11:43:07Z

(Apologies for the force push; I mistakenly added commits to this PR that was intended to go on another branch)

openjdk · 2024-03-13T11:50:08Z

@magicus 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.

magicus · 2024-03-13T11:53:47Z

/label core-libs

openjdk · 2024-03-13T11:54:50Z

@magicus
The core-libs label was successfully added.

magicus · 2024-03-13T11:56:31Z

I tagged this core-libs as that is what @sviswa7 did for changes to libjsvml in #8508. I hope this is correct. (We should really add rules to Skara for this library.)

openjdk · 2024-03-13T13:42:58Z

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

8328074: Add jcheck whitespace checking for assembly files

Reviewed-by: erikj, sviswanathan

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

0204aac: 8328115: Convert java/awt/font/TextLayout/TestJustification.html applet test to main
9bc1b06: 8328242: Add a log area to the PassFailJFrame
65a84c2: 8328006: refactor large anonymous inner class in HtmlDocletWriter
044f4ed: 8326979: (jdeps) improve the error message for FindException caused by InvalidModuleDescriptorException
71f9c4e: 8320534: fatal error for the NMTBenchmark test run for the mainline build
bfba489: 8328157: Move C[XX]FLAGS_JDK[LIB/EXE] to JdkNativeCompilation.gmk
42aec8c: 8328146: Set LIBCXX automatically
90ab52e: 8328166: Epsilon: 'EpsilonHeap::allocate_work' misuses the parameter 'size' as size in bytes
bf990c8: 8328269: NonFocusablePopupMenuTest.java should be marked as headful
80b98c8: 8320503: Print warning if VM reaches MallocLimit during error reporting
... and 67 more: https://git.openjdk.org/jdk/compare/49d8008947534898e2ba36e2d81d87d6ae4b4c02...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.

magicus · 2024-03-15T14:51:14Z

Maybe someone in Hotspot wants to take a look?
/label +hotspot

openjdk · 2024-03-15T14:52:09Z

@magicus
The hotspot label was successfully added.

sviswa7

Looks good to me.

magicus · 2024-03-18T09:11:09Z

Thanks @sviswa7!

/integrate

openjdk · 2024-03-18T09:12:42Z

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

3c70f26: 8328112: Remove CardTable::_guard_region
48717d6: 8326333: jshell completion on arrays is incomplete
ece4124: 8328247: Remove redundant dir for tests converted from applet to main
d32ce65: 8327651: Rename DictionaryEntry members related to protection domain
0719419: 8328236: module_entry in CDS map file has stale value
0204aac: 8328115: Convert java/awt/font/TextLayout/TestJustification.html applet test to main
9bc1b06: 8328242: Add a log area to the PassFailJFrame
65a84c2: 8328006: refactor large anonymous inner class in HtmlDocletWriter
044f4ed: 8326979: (jdeps) improve the error message for FindException caused by InvalidModuleDescriptorException
71f9c4e: 8320534: fatal error for the NMTBenchmark test run for the mainline build
... and 72 more: https://git.openjdk.org/jdk/compare/49d8008947534898e2ba36e2d81d87d6ae4b4c02...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-03-18T09:12:50Z

@magicus Pushed as commit c342188.

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

magicus added 2 commits March 13, 2024 12:01

Run expand on libjsvml

df42668

Enable jcheck whitespace checks for .S files

ce1d4c9

openjdk bot added the rfr Pull request is ready for review label Mar 13, 2024

openjdk bot added the build build-dev@openjdk.org label Mar 13, 2024

TheShermanTanker reviewed Mar 13, 2024

View reviewed changes

magicus force-pushed the fix-jcheck-assembly-files branch from 49fb101 to ce1d4c9 Compare March 13, 2024 11:42

Make all ALIGN/.align aligned

4e729cb

openjdk bot added the core-libs core-libs-dev@openjdk.org label Mar 13, 2024

erikj79 approved these changes Mar 13, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 13, 2024

openjdk bot added the hotspot hotspot-dev@openjdk.org label Mar 15, 2024

sviswa7 approved these changes Mar 16, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Mar 18, 2024

openjdk bot closed this Mar 18, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 18, 2024

magicus deleted the fix-jcheck-assembly-files branch March 18, 2024 10:06

nizarbenalla mentioned this pull request May 10, 2024

8144100: Incorrect case-sensitive equality in com.sun.net.httpserver.BasicAuthenticator #19133

Closed

3 tasks

8328074: Add jcheck whitespace checking for assembly files #18268

8328074: Add jcheck whitespace checking for assembly files #18268

Uh oh!

Conversation

magicus commented Mar 13, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Mar 13, 2024

Uh oh!

openjdk bot commented Mar 13, 2024

Uh oh!

mlbridge bot commented Mar 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

TheShermanTanker Mar 13, 2024

Choose a reason for hiding this comment

Uh oh!

magicus Mar 13, 2024

Choose a reason for hiding this comment

Uh oh!

magicus commented Mar 13, 2024

Uh oh!

openjdk bot commented Mar 13, 2024

Uh oh!

magicus commented Mar 13, 2024

Uh oh!

openjdk bot commented Mar 13, 2024

Uh oh!

magicus commented Mar 13, 2024

Uh oh!

openjdk bot commented Mar 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

magicus commented Mar 15, 2024

Uh oh!

openjdk bot commented Mar 15, 2024

Uh oh!

sviswa7 left a comment

Choose a reason for hiding this comment

Uh oh!

magicus commented Mar 18, 2024

Uh oh!

openjdk bot commented Mar 18, 2024

Uh oh!

openjdk bot commented Mar 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

magicus commented Mar 13, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 13, 2024 •

edited

Loading

openjdk bot commented Mar 13, 2024 •

edited

Loading