ci: add MSan Linux job - #950
Conversation
|
An automated preview of the documentation is available at https://950.mrdocs.prtest2.cppalliance.org/index.html |
1 similar comment
|
An automated preview of the documentation is available at https://950.mrdocs.prtest2.cppalliance.org/index.html |
|
An automated preview of the documentation is available at https://950.mrdocs.prtest2.cppalliance.org/index.html |
|
An automated preview of the documentation is available at https://950.mrdocs.prtest2.cppalliance.org/index.html |
e5fd260 to
ce365cb
Compare
|
An automated preview of the documentation is available at https://950.mrdocs.prtest2.cppalliance.org/index.html |
ce365cb to
b9c1aa0
Compare
|
An automated preview of the documentation is available at https://950.mrdocs.prtest2.cppalliance.org/index.html |
b9c1aa0 to
e89e7b7
Compare
|
An automated preview of the documentation is available at https://950.mrdocs.prtest2.cppalliance.org/index.html |
e89e7b7 to
b97485b
Compare
|
An automated preview of the documentation is available at https://950.mrdocs.prtest2.cppalliance.org/index.html |
b97485b to
1e2c460
Compare
|
An automated preview of the documentation is available at https://950.mrdocs.prtest2.cppalliance.org/index.html |
1e2c460 to
80bdc65
Compare
|
An automated preview of the documentation is available at https://950.mrdocs.prtest2.cppalliance.org/index.html |
204baa9 to
7cd0ed0
Compare
|
An automated preview of the documentation is available at https://950.mrdocs.prtest2.cppalliance.org/index.html |
1 similar comment
|
An automated preview of the documentation is available at https://950.mrdocs.prtest2.cppalliance.org/index.html |
7cd0ed0 to
30b4802
Compare
|
An automated preview of the documentation is available at https://950.mrdocs.prtest2.cppalliance.org/index.html |
|
An automated preview of the documentation is available at https://950.mrdocs.prtest2.cppalliance.org/index.html |
4 similar comments
|
An automated preview of the documentation is available at https://950.mrdocs.prtest2.cppalliance.org/index.html |
|
An automated preview of the documentation is available at https://950.mrdocs.prtest2.cppalliance.org/index.html |
|
An automated preview of the documentation is available at https://950.mrdocs.prtest2.cppalliance.org/index.html |
|
An automated preview of the documentation is available at https://950.mrdocs.prtest2.cppalliance.org/index.html |
| extra-values: | | ||
| llvm-hash: a1b6e7ff393533a5c4f3bdfd4efe5da106e2de2b | ||
| llvm-build-preset-prefix: {{#if optimized-debug}}debwithopt{{else}}{{{lowercase build-type}}}{{/if}} | ||
| use-libcxx: {{#if (and (ieq compiler 'clang') (or (eq major 19) (eq major 20) (eq major 21))) }}true{{else}}false{{/if}} |
There was a problem hiding this comment.
I'm adding this gt feature now. That logic is going to break soon. Especially because it says nothing about sanitizers. It's only relying on the version.
There was a problem hiding this comment.
I think it makes sense to also use the split libcxx for the clang non-sanitizer version, because that would be a different test checking we build correctly with libc++ instead of libstdc++, and if there is any problems there we will both get the answer faster, and the isolated error without any sanitizer involvement.
There was a problem hiding this comment.
If that's the case, then you don't even need to check major. You just have to check if it's clang.
There was a problem hiding this comment.
But that can't work with clang-18, as libc++ has already removed support for compiling with it.
| echo -E "llvm-root=$llvm_root" >> $GITHUB_OUTPUT | ||
| echo "deb http://apt.llvm.org/noble/ llvm-toolchain-noble-21 main" >> /etc/apt/sources.list | ||
| apt-get update | ||
| apt-get install -y libclang-18-dev libclang-19-dev libclang-20-dev libclang-21-dev |
There was a problem hiding this comment.
We need all of these versions?
There was a problem hiding this comment.
Yes, there is a bug in how the llvm-dev package is split from the clang-dev only one, where some of the cmake module code errors out.
This happens when configuring the standalone libc++ build, where it tries to find the llvm cmake module, calls something into it, but that errors out because it can't find some cmake files which are only shipped in the clang-dev package.
There was a problem hiding this comment.
Maybe it's because I don't understand the implications of this bug. However, I have no idea what the connection is between this bug and the answer to the question I asked about why we needed all these versions. Is it a bug in the CMake module that randomly gets fixed when you install multiple equally buggy versions simultaneously?
There was a problem hiding this comment.
Ah, it's all the clang versions we compile libc++ with. Regarding versions, I'll remove clang-18 since it's unsupported anyway.
| echo -E "llvm-root=$llvm_root" >> $GITHUB_OUTPUT | ||
| echo "deb http://apt.llvm.org/noble/ llvm-toolchain-noble-21 main" >> /etc/apt/sources.list | ||
| apt-get update | ||
| apt-get install -y libclang-18-dev libclang-19-dev libclang-20-dev libclang-21-dev |
There was a problem hiding this comment.
We already have a step for installing packages and the matrix already sets the packages we need. Do we need to implement any feature in the actions to make this work there?
There was a problem hiding this comment.
I think the main problem is that we need to insert the llvm repo here.
If the action adds support for that, then we can eliminate this.
There was a problem hiding this comment.
Have you been able to check the documentation of the package-install action, or are you assuming it doesn't have this feature? IIRC, it does have this feature because we needed it before. We can also include it if necessary, as it would simply be a loop over some keys. But I believe the feature is already there.
| contents: write | ||
|
|
||
| steps: | ||
| - name: Resolved Matrix |
There was a problem hiding this comment.
So maybe we could have a step to resolve paths. I think this already exists for a single path at a time, though. Or it could be an action to take another dictionary and allow lots of operations on the values, like resolving paths, concatenating strings or whatever.
alandefreitas
left a comment
alandefreitas
left a comment
There was a problem hiding this comment.
Well... bootstrap. :)
| void | ||
| buildTerminal( | ||
| NestedNameSpecifier const*, | ||
| NestedNameSpecifier, |
There was a problem hiding this comment.
These changes are fixes MSan trigger? The calling code just worked unchanged? I smell something.
There was a problem hiding this comment.
I missed those changes from the LLVM version bump I did a while ago.
I don't understand how the code compiled without those changes, but with the new jobs we started seeing linker errors because of this.
So yes, I think there is some deeper issue here, I just didn't look into it yet.
Yeah, as we talked about this, I assume there will be no problem if I post this on a separate PR, as these are orthogonal things we don't need to mix up in one commit. |
|
An automated preview of the documentation is available at https://950.mrdocs.prtest2.cppalliance.org/index.html |
4 similar comments
|
An automated preview of the documentation is available at https://950.mrdocs.prtest2.cppalliance.org/index.html |
|
An automated preview of the documentation is available at https://950.mrdocs.prtest2.cppalliance.org/index.html |
|
An automated preview of the documentation is available at https://950.mrdocs.prtest2.cppalliance.org/index.html |
|
An automated preview of the documentation is available at https://950.mrdocs.prtest2.cppalliance.org/index.html |
Only if bootstrap is not a source of truth, which it attempts to be In fact, one future plan is to replace literally everything in CI with a single call to bootstrap.py |
|
An automated preview of the documentation is available at https://950.mrdocs.prtest2.cppalliance.org/index.html |
@sdarwin, is it possible to tell the bot to only make this comment once per PR? |
|
An automated preview of the documentation is available at https://950.mrdocs.prtest2.cppalliance.org/index.html |
|
@alandefreitas reviewing the available settings in Jenkins, there is a possibility: "Ignore Pull Requests marked as Drafts". Coincidentally, a pull request that's in very active development, getting 100 updates, is probably a "draft", right? It's not ready to merge. |
It's not possible to turn PRs back into drafts on GitHub. FWIW this shouldn't have to rely on that, specially in this case where the url doesn't even change. I have seen similar bot actions, where even when there is an actual update, the bot just updates the existing comment in place, instead of adding a new comment. |
It's now a draft. |
|
TIL you can do that, where is the option? |
Not sure what your current collaboration settings are, if "administrator" (as Alan) there's a "convert to draft" setting near "Reviews". |
Phew |
4 participants
Adds MSan sanitizer job.
This builds libc++, if the host is capable of it and for non-release jobs, and uses it instead of the system library.
This libc++ will be built with whatever sanitizer is selected, increasing the effectiveness of those tests.
This is also a requirement for MSan, which requires everything to be compiled with it.
Building the libc++ separately, with the host compiler, also decreases the maximum amount of disk space the CI jobs need.
In terms of host compiler, the libc++ being used requires [apple-]clang >= 19 or GCC 15. The other compilers are still built in the pre-existing manner, using the bootstrap build.
For now, GCC 15 doesn't use this yet, because there is some issue linking the library which needs to be investigated.
apple-clang, when built with sanitizers, also has a special requirement which is not being met, so is also not opting into the standalone libc++ build. This is left to improve later.