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 toolchain to Clang 17 #8537

Merged
merged 20 commits into from
Apr 2, 2024

Conversation

JaySon-Huang
Copy link
Contributor

@JaySon-Huang JaySon-Huang commented Dec 17, 2023

What problem does this PR solve?

Issue Number: close #7193, close #8890

Problem Summary:

What is changed and how it works?

Use clang-17.0.6 for CI/CD building since this PR merged. Other branches keep using clang-13.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

tpch50, 1 tidb, 1 tikv, 1 tiflash, https://github.com/pingcap/tiflash/tree/c15fcc2bf83c6e1230c43a0daaccdc00fb15304f

Query  clang13 (s)   clang17 (s)
Q1      1.71            1.71
Q2      0.85            0.83
Q3      1.51            1.47
Q4      5.39            4.46
Q5      2.87            2.79
Q6      0.44            0.43
Q7      1.59            1.58
Q8      2.24            2.21
Q9      8.23            7.95
Q10     1.91            1.87
Q11     0.57            0.62
Q12     1.02            1.07
Q13     2.09            2.13
Q14     0.57            0.57
Q15     1.21            1.23
Q16     0.50            0.53
Q17     4.33            4.41
Q18     3.87            3.91
Q19     0.93            0.84
Q20     0.89            0.85
Q21     3.55            3.46
Q22     0.35            0.34
Total  49.06           48.28

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

None

@ti-chi-bot ti-chi-bot bot added do-not-merge/needs-linked-issue release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 17, 2023
@purelind
Copy link
Collaborator

Hi, because here specify image_tag_suffix in .toolchain.yml, to ensure that build-common can pull images properly, need to confirm if this image hub.pingcap.net/tiflash/tiflash-llvm-base:amd64-llvm-14.0.6 exists, if not, need to build the image first and upload it.

ci groovy file https://github.com/PingCAP-QE/ci/blob/main/jenkins/pipelines/ci/tiflash/tiflash-build-common.groovy#L453

@ti-chi-bot ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 18, 2023
@JaySon-Huang JaySon-Huang self-assigned this Dec 27, 2023
@ti-chi-bot ti-chi-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 27, 2023
@ti-chi-bot ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 30, 2023
ti-chi-bot bot pushed a commit to PingCAP-QE/ci that referenced this pull request Jan 2, 2024
Try to build tiflash using clang-17 docker image
pingcap/tiflash#8537

But the CI will run into errors like
https://ci.pingcap.net/job/tiflash-build-common/33843/console
```
[2023-12-30T15:06:42.887Z] + git log -1 --format=%H
[2023-12-30T15:06:42.887Z] fatal: detected dubious ownership in repository at '/home/jenkins/agent/workspace/tiflash-build-common/tiflash/contrib/tiflash-proxy'
[2023-12-30T15:06:42.887Z] To add an exception for this directory, call:
```
```
23:16:59  fatal: detected dubious ownership in repository at '/home/jenkins/agent/workspace/tiflash-build-common/tiflash'
23:16:59  To add an exception for this directory, call:
23:16:59  
23:16:59  	git config --global --add safe.directory /home/jenkins/agent/workspace/tiflash-build-common/tiflash
23:16:59  CMake Error at dbms/cmake/version.cmake:20 (execute_process):
23:16:59    execute_process failed command indexes:
23:16:59  
23:16:59      1: "Child return code: 128"
23:16:59  
23:16:59  Call Stack (most recent call first):
23:16:59    dbms/CMakeLists.txt:25 (include)
```

Add git safe.directory to workaround the errors. After these changes,
the pipeline can build tiflash with clang-17 successfully:
https://ci.pingcap.net/job/tiflash-build-common/33844/consoleFull

Signed-off-by: JaySon-Huang <tshent@qq.com>
@ti-chi-bot ti-chi-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed do-not-merge/needs-linked-issue size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 2, 2024
JaySon-Huang and others added 9 commits January 5, 2024 15:48
Signed-off-by: JaySon-Huang <tshent@qq.com>
Signed-off-by: SchrodingerZhu <i@zhuyi.fan>
Signed-off-by: SchrodingerZhu <i@zhuyi.fan>
Signed-off-by: JaySon-Huang <tshent@qq.com>
Copy link
Contributor

@Lloyd-Pottiger Lloyd-Pottiger left a comment

Choose a reason for hiding this comment

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

image image

I advise update to clang-17 both llvm and apple uniformly.

Others LGTM

@ti-chi-bot ti-chi-bot bot added the lgtm label Apr 1, 2024
Copy link
Contributor

ti-chi-bot bot commented Apr 1, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JinheLin, Lloyd-Pottiger

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [JinheLin,Lloyd-Pottiger]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot bot removed the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Apr 1, 2024
Copy link
Contributor

ti-chi-bot bot commented Apr 1, 2024

[LGTM Timeline notifier]

Timeline:

  • 2024-04-01 09:58:20.135795831 +0000 UTC m=+265161.663336369: ☑️ agreed by JinheLin.
  • 2024-04-01 12:49:59.137510915 +0000 UTC m=+275460.665051501: ☑️ agreed by Lloyd-Pottiger.

Signed-off-by: JaySon-Huang <tshent@qq.com>
Signed-off-by: JaySon-Huang <tshent@qq.com>
@JaySon-Huang
Copy link
Contributor Author

JaySon-Huang commented Apr 1, 2024

The latest MacOS 14.4.1 only provides clang 15 by default, so I keep require AppleClang 15.0.0+ by now

@JaySon-Huang
Copy link
Contributor Author

/run-all-tests

@Lloyd-Pottiger Lloyd-Pottiger changed the title Clang 17 support test Update toolchain to Clang 17 Apr 2, 2024
@JinheLin
Copy link
Contributor

JinheLin commented Apr 2, 2024

[2024-04-02T05:18:10.602Z] [2463/2479] TestMode/DeltaMergeStoreRWTest.DMFileNameChangedInDMFileReadPool/V3_FileOnly (4628 ms)
[2024-04-02T05:18:14.764Z] [2464/2479] TestMode/DeltaMergeStoreRWTest.DMFileNameChangedInDMFileReadPool/V3_Mix (4605 ms)
[2024-04-02T05:21:51.058Z] Sending interrupt signal to process
[2024-04-02T05:21:51.059Z] Killing processes
[2024-04-02T05:21:51.414Z] kill finished with exit code 0
[2024-04-02T05:22:06.171Z] script returned exit code 143
[2024-04-02T05:22:06.301Z] org.jenkinsci.plugins.workflow.steps.FlowInterruptedException

Seems timeout.

@JinheLin
Copy link
Contributor

JinheLin commented Apr 2, 2024

/run-unit-test

@JaySon-Huang
Copy link
Contributor Author

JaySon-Huang commented Apr 2, 2024

/hold
some ABI is changed in clang-17, so __PRETTY_FUNCTION__ is changed. Causing some ut failure

@ti-chi-bot ti-chi-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 2, 2024
@JaySon-Huang
Copy link
Contributor Author

/unhold

@ti-chi-bot ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 2, 2024
@JaySon-Huang
Copy link
Contributor Author

/run-all-tests

Copy link
Contributor

@Lloyd-Pottiger Lloyd-Pottiger left a comment

Choose a reason for hiding this comment

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

image

nit: we should upgrade the ninja in the docker.

@JaySon-Huang
Copy link
Contributor Author

/run-unit-test

@ti-chi-bot ti-chi-bot bot merged commit 52fc334 into pingcap:master Apr 2, 2024
6 checks passed
@JaySon-Huang JaySon-Huang deleted the clang-17-support-test branch April 2, 2024 10:24
@ti-chi-bot ti-chi-bot bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Apr 2, 2024
@sre-bot
Copy link
Collaborator

sre-bot commented Apr 2, 2024

@sre-bot
Copy link
Collaborator

sre-bot commented Apr 2, 2024

@ti-chi-bot ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

unstable test RegionKVStoreOldTest.RegionRange Clang 17 & C++23 Compatibility
6 participants