Let CI's sanitizer job use clang's default version#13219
Merged
shindere merged 1 commit intoocaml:trunkfrom Jul 19, 2024
Merged
Let CI's sanitizer job use clang's default version#13219shindere merged 1 commit intoocaml:trunkfrom
shindere merged 1 commit intoocaml:trunkfrom
Conversation
MisterDA
approved these changes
Jun 5, 2024
Contributor
MisterDA
left a comment
There was a problem hiding this comment.
The end of the script is commented out but could be updated too. I suggest to name the variable llvm_version.
5053922 to
81e91b0
Compare
Contributor
Author
|
Thanks a lot for the two suggestions, that I did take into account.
I didn't dare to modify the commented bit because it was referring to a
different llvm version, but it's probably better to do that and be
homogeneous.
I also had to slightly shorten the right bannter in the first invocation
of `echo` to keep the line length below 80 characters so I went ahead
and also shortened the other right bannbters form 10 `=` signs to 8, for
consistency between the different `echo` commands and also with the
length of the left banner.
|
This is to avoid having to maintain a hard-coded version
81e91b0 to
5d16740
Compare
MisterDA
approved these changes
Jun 5, 2024
Contributor
|
Much neater, thanks! |
OlivierNicole
approved these changes
Jun 5, 2024
Contributor
OlivierNicole
left a comment
There was a problem hiding this comment.
Looks correct to me too. I don’t know why version 14 (and even 9) are hardcoded, but if there’s no good reason, I’m all for moving to a more recent and regularly updated version.
Contributor
Author
|
Olivier Nicole (2024/06/05 05:58 -0700):
@OlivierNicole approved this pull request.
Thanks!
I don’t know why version 14 (and even 9) are hardcoded, but if there’s
no good reason, I’m all for moving to a more recent and regularly
updated version.
My understanding is that one had to know which version we were using so
that one could use the right `llvm-symbolizer`.
So the version had to be explixcit, and I think 14 and 9 were those
which were state of the art at the time.
As I understand it, `ASAN_SYMBOLIZER_PATH` and `TSAN_SYMBOLIZER_PATH`
MUST contain the path of the tool. Them being undefined just means
`don't use the tool at all` rather than `use the tool of the same LLVM
version`. Sadly.
|
Contributor
|
#13290 will also modify the script. I think this PR should be merged before. I don't see anything holding it back. |
Contributor
Do you have
I can rebase my PRs once this one is merged. |
This was referenced Jul 12, 2024
dra27
pushed a commit
that referenced
this pull request
Mar 16, 2026
Let CI's sanitizer job use clang's default version (cherry picked from commit f4b13c2)
dra27
pushed a commit
that referenced
this pull request
Mar 16, 2026
Let CI's sanitizer job use clang's default version (cherry picked from commit f4b13c2)
dra27
pushed a commit
that referenced
this pull request
Mar 16, 2026
Let CI's sanitizer job use clang's default version (cherry picked from commit f4b13c2)
dra27
pushed a commit
that referenced
this pull request
Mar 16, 2026
Let CI's sanitizer job use clang's default version (cherry picked from commit f4b13c2)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is to avoid having to maintain a hard-coded version number
@xavierleroy may like this. Also discussed recenty with both @MisterDA and @OlivierNicole.
It may be interesting to backport this to other release branches, which
would make it possible to uninstall the older clang versions from the
server. @Octachron may have an opinion on this.