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

Update JIT sources to clang-format/clang-tidy 17.0.6 #100498

Merged
merged 3 commits into from
Apr 3, 2024

Conversation

BruceForstall
Copy link
Member

The version of clang-format/clang-tidy we use in the JIT is very old and needs to be updated: see #59545. One reason is to allow support for jit-format on arm64 Mac, and possibly arm64 Linux. Another is simply to better support newer language features and fix bugs.

This PR shows an updated clang-format configuration file that gets us about as close as possible to the formatting we currently have (it is impossible to get it 100% the same). There are some nice improvements here.

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Apr 1, 2024
@BruceForstall
Copy link
Member Author

I verified that I could jit-format on Windows x64, then jit-format on Mac arm64, and no additional formatting changes were made.

@BruceForstall
Copy link
Member Author

@dotnet/jit-contrib @jkoritzinsky PTAL

@BruceForstall
Copy link
Member Author

Related: #71983

cc @a74nh

Looks like the 17.0.6 release has some aarch64 Linux binaries: https://github.com/llvm/llvm-project/releases/tag/llvmorg-17.0.6

@jkoritzinsky
Copy link
Member

Do we get any benefits with LLVM 17 over 16? I'm asking as the rest of the LLVM build tool usages in our repo are on v16 currently and it would be nice to align them (and upgrade them in tandem).

@BruceForstall
Copy link
Member Author

I'd rather go to the newest possible, and 17.0.6 was the latest that also had built Ubuntu binaries (as well as osx-arm64, windows-x64). I don't think we should tie the JIT use of clang-format/clang-tidy to any other use of LLVM; that just complicates dependencies and our ability to update.

@BruceForstall
Copy link
Member Author

Note that this change requires dotnet/jitutils#408.

All the pieces are in place where we could take this change now. I've tested jit-format on linux-x64 (Ubuntu 22.04.4 LTS), win-x64, and osx-arm64.

@BruceForstall BruceForstall marked this pull request as ready for review April 2, 2024 01:19
@BruceForstall BruceForstall force-pushed the ClangFormat17.0.6_Win branch from 30ca3b6 to 8ee3a98 Compare April 2, 2024 17:24
@BruceForstall
Copy link
Member Author

I updated the formatted code if you want to look, or look again.

@SingleAccretion
Copy link
Contributor

I have went through the entirety of the change. The majority of it look great. In particular, it looks that it will now be possible to delete the CLANG_FORMAT_ANCHOR workaround.

There were, however, three patterns that looked problematic:
1) Code comments align together with those from #ifdefs, e. g.:

 #else  // !TARGET_ARM64
             // There is no zero register on ARM32
+       // There is no zero register on ARM32
             unreached();
 #endif // !TARGET_ARM64

I skimmed the Clang format document, it didn't look like it has specific options for this situation (comment aligning: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#aligntrailingcomments). Perhaps, this can be worked around in source with a whitespace:

 #else  // !TARGET_ARM64
+
             // There is no zero register on ARM32
             unreached();
 #endif // !TARGET_ARM64

Or deleting the ifdef comments in case of very short ifdefs.
2) Code aligns together with an if that is meant to be conditionally else if:

 #ifdef TARGET_XYZ
 if (...)
 {
     ...
 }
 else
 #endif // TARGET_XYZ
     if (...)
-{
-    ...
-}
+   {
+       ...
+   }

Not sure how to fix this. Maybe the by unindenting the second if manually?

 #endif // TARGET_XYZ
-    if (...)
+if (...)
 {
     ...
 }

3) Long comments on macros induce multi-line macros:

-#define CHECK_STRUCT_PADDING 0 // Set this to '1' to enable warning C4820 "'bytes' bytes padding added after
-                               // construct 'member_name'" on interesting structs/classes
+#define CHECK_STRUCT_PADDING                                                                                           \
+    0 // Set this to '1' to enable warning C4820 "'bytes' bytes padding added after
+      // construct 'member_name'" on interesting structs/classes

I guess it is not a big deal, but it does look worse.

There is also one miscellaneous change: closing braces after namespaces now get a comment:

-}
+} // namespace XYZ

Do we want it? (Can be turned off via https://clang.llvm.org/docs/ClangFormatStyleOptions.html#fixnamespacecomments).

@BruceForstall
Copy link
Member Author

@SingleAccretion Thanks for looking!

  1. Code comments align together with those from #ifdef

That one's odd. I think we'll have to work around it with additional whitespace.

  1. Code aligns together with an if that is meant to be conditionally else if:

I think the best way to fix this is to avoid having "else" and "if" in separate #ifdef blocks, which IMO is already a readability problem.

  1. Long comments on macros induce multi-line macros:

It doesn't happen too often, and I think the fix is to put non-trivial (i.e., multi-line) comments before the macros, e.g.,

// Set this to '1' to enable warning C4820 "'bytes' bytes padding added after
// construct 'member_name'" on interesting structs/classes
#define CHECK_STRUCT_PADDING 0

Related: I'd like to reformat a lot of jitconfigvalues.h this way, as it has far too many weirdly-formatted and multi-line comments.

There is also one miscellaneous change: closing braces after namespaces now get a comment:

Looks useful to me (I hadn't notice that one; we don't use namespaces too often).

@BruceForstall
Copy link
Member Author

This is ready to go. It requires dotnet/jitutils#408 as well.

I tested the CI format job with #100565, which is the same as this PR but pulled the jitutils bootstrap.cmd/sh from my jitutils PR branch.

Note that osx-x64 is no longer supported for formatting (but osx-arm64 is!). (It might be possible if you build your own clang 17.0.6 osx-x64 clang-format/clang-tidy?)

I'd like someone to pull down this and the corresponding jitutils PR on an osx-arm64 machine to verify it works for them, as well.

@AndyAyersMS @jakobbotsch @dotnet/jit-contrib PTAL

Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

Thanks for doing this. The new format looks better.

@BruceForstall BruceForstall force-pushed the ClangFormat17.0.6_Win branch from b52931c to b173759 Compare April 3, 2024 17:51
@BruceForstall
Copy link
Member Author

/ba-g Formatting job failures are expected due to required cross-repo coordinated checkin

@BruceForstall BruceForstall merged commit 862c82f into dotnet:main Apr 3, 2024
172 of 179 checks passed
@BruceForstall BruceForstall deleted the ClangFormat17.0.6_Win branch April 3, 2024 21:43
BruceForstall added a commit to BruceForstall/runtime that referenced this pull request Apr 3, 2024
BruceForstall added a commit that referenced this pull request Apr 4, 2024
* Remove CLANG_FORMAT_COMMENT_ANCHOR

Not needed after #100498

* Fix build

* Formatting
matouskozak pushed a commit to matouskozak/runtime that referenced this pull request Apr 30, 2024
* Update JIT sources to clang-format/clang-tidy 17.0.6

* Reformat

* Reformat x86
matouskozak pushed a commit to matouskozak/runtime that referenced this pull request Apr 30, 2024
* Remove CLANG_FORMAT_COMMENT_ANCHOR

Not needed after dotnet#100498

* Fix build

* Formatting
@github-actions github-actions bot locked and limited conversation to collaborators May 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants