[tools] Use mold linker on Linux debug builds #22951
Conversation
rpoyner-tri
force-pushed
the
mold-integration
branch
from
d3a26fb to
f66437a
Compare
There was a problem hiding this comment.
tons of builds will fail without revised provisioning. The goal at this point is for the correct set of builds to fail.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", missing label for release notes (waiting on @rpoyner-tri)
|
@drake-jenkins-bot linux-jammy-unprovisioned-clang-bazel-experimental-debug please. |
rpoyner-tri
force-pushed
the
mold-integration
branch
from
f66437a to
f2c146e
Compare
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
@drake-jenkins-bot linux-jammy-unprovisioned-clang-bazel-experimental-debug please. |
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
@drake-jenkins-bot linux-noble-unprovisioned-clang-bazel-experimental-debug please. |
There was a problem hiding this comment.
We are now at an interesting spot. Pretty much the correct sets of builds pass and fail, and for the right reasons (provisioned debugs fail for lack of requested mold linker). The outlier is unprovisioned debug on jammy; the old version of mold in Jammy is not good enough to pass the install test. I will claim this maybe doesn't matter, and perhaps the plan should be to disable that test when config is (Jammy AND -compilation-mode=dbg).
The version of mold in noble is (recently?) considerably newer, and handles install test just fine.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", missing label for release notes (waiting on @rpoyner-tri)
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
@drake-jenkins-bot linux-jammy-unprovisioned-gcc-bazel-experimental-debug please. |
rpoyner-tri
force-pushed
the
mold-integration
branch
3 times, most recently
from
c688b02 to
b8d155a
Compare
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
@drake-jenkins-bot linux-noble-unprovisioned-clang-bazel-experimental-debug please. |
rpoyner-tri
force-pushed
the
mold-integration
branch
2 times, most recently
from
dd016ff to
999792f
Compare
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
@drake-jenkins-bot linux-noble-unprovisioned-clang-bazel-experimental-debug please. |
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
@drake-jenkins-bot linux-noble-unprovisioned-clang-bazel-experimental-debug please. |
1 similar comment
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
@drake-jenkins-bot linux-noble-unprovisioned-clang-bazel-experimental-debug please. |
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
@drake-jenkins-bot linux-noble-unprovisioned-clang-bazel-experimental-debug please. |
There was a problem hiding this comment.
r2 disabled some tests for jammy and dbg. Now the only build failing that should not is
https://drake-jenkins.csail.mit.edu/job/linux-jammy-unprovisioned-gcc-bazel-experimental-debug/27/
which is reported "no space left on device".
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", missing label for release notes (waiting on @rpoyner-tri)
rpoyner-tri
force-pushed
the
mold-integration
branch
from
999792f to
e5933ee
Compare
There was a problem hiding this comment.
Latest (still r2 according to reviewable) abandons jammy mold.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", missing label for release notes (waiting on @rpoyner-tri)
There was a problem hiding this comment.
-(status: do not review) +@jwnimmer-tri for a first read.
Still not-merge, since this will need Noble CI provisioning updates.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", missing label for release notes (waiting on @rpoyner-tri)
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
@drake-jenkins-bot linux-noble-unprovisioned-clang-bazel-experimental-debug please. |
There was a problem hiding this comment.
Reviewed 1 of 4 files at r2.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", missing label for release notes (waiting on @rpoyner-tri)
setup/ubuntu/source_distribution/packages-noble-test-only.txt line 3 at r2 (raw file):
curl libstdc++6-10-dbg mold
Users who build Drake from source via CMake still end up using -c dbg because we plumb the CMake "Debug" option through to -c dbg in our CMake-Bazel wrapper. Thus, those users will now get an error message about not having mold, since they don't enable "test only" dependencies.
Do we want downstream project consuming Drake as an external to use mold, or should it only be used for Drake Developers? That will decide how we go about re-aligning things.
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", missing label for release notes (waiting on @rpoyner-tri)
setup/ubuntu/source_distribution/packages-noble-test-only.txt line 3 at r2 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Users who build Drake from source via CMake still end up using
-c dbgbecause we plumb the CMake "Debug" option through to-c dbgin our CMake-Bazel wrapper. Thus, those users will now get an error message about not having mold, since they don't enable "test only" dependencies.Do we want downstream project consuming Drake as an external to use mold, or should it only be used for Drake Developers? That will decide how we go about re-aligning things.
It's a good question. Right now we don't have an alternate story for getting gdb line numbers to work, but maybe we don't care about that w.r.t. downstream projects.
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", missing label for release notes (waiting on @rpoyner-tri)
setup/ubuntu/source_distribution/packages-noble-test-only.txt line 3 at r2 (raw file):
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
It's a good question. Right now we don't have an alternate story for getting gdb line numbers to work, but maybe we don't care about that w.r.t. downstream projects.
Having a broken build is worse that missing line numbers. Since it's probably somewhat difficult to engineer this to work correctly when Drake is used as a CMake external, at the very least mold should be Bazel-only, and probably even Developer-only.
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
@drake-jenkins-bot linux-noble-unprovisioned-clang-bazel-experimental-debug please. |
There was a problem hiding this comment.
LGTM on paper but I'll want to test it locally for Developer UX once I'm back on my workstation.
Reviewed 1 of 4 files at r2, 1 of 3 files at r3, 2 of 2 files at r5, all commit messages.
Reviewable status: 7 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", missing label for release notes (waiting on @rpoyner-tri)
tools/skylark/BUILD.bazel line 18 at r5 (raw file):
) config_setting(
Our configs for C++ toolchain (compilers, dbg, etc) currently mostly live in //tools/cc_toolchain. How about moving all of this new BUILD content into there as well?
tools/skylark/BUILD.bazel line 23 at r5 (raw file):
) bool_flag(
All flags need API documentation. (See examples in cc_toolchain.) I imagine one part of the docs will be "# This is intended for internal use only, not for users to configure." since we don't want this to be public?
tools/skylark/drake_cc.bzl line 800 at r5 (raw file):
srcs = srcs, deps = deps, copts = new_copts,
Missing new_linkopts.
For any functions where (like this one) where we stole linkopts out of kwargs, we need to pass in new_linkopts everywhere kwargs was previously passed.
(In this case, yes, for now the new library doesn't have any srcs so linkopts is most likely irrelevant, but we shouldn't leave traps around for future edits.)
tools/skylark/drake_cc.bzl line 832 at r5 (raw file):
data = data + test_rule_data, deps = deps + add_deps, copts = copts,
Missing new_linkopts here.
tools/skylark/drake_cc.bzl line 888 at r5 (raw file):
deps = deps, copts = new_copts, **kwargs
Missing new_linkopts
tools/lint/library_lint_reporter.py line 67 at r5 (raw file):
if not (item.startswith("//tools/cc_toolchain:") or "@bazel_tools//" in item or item.startswith("//tools/skylark:"))
BTW After moving the BUILD stuff as suggested below, this file can also be revered.
rpoyner-tri
force-pushed
the
mold-integration
branch
from
701ee5f to
544e206
Compare
There was a problem hiding this comment.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", missing label for release notes
tools/skylark/drake_cc.bzl line 800 at r5 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Missing new_linkopts.
For any functions where (like this one) where we stole linkopts out of kwargs, we need to pass in new_linkopts everywhere kwargs was previously passed.
(In this case, yes, for now the new library doesn't have any srcs so linkopts is most likely irrelevant, but we shouldn't leave traps around for future edits.)
Done.
tools/skylark/drake_cc.bzl line 832 at r5 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Missing new_linkopts here.
Done.
tools/skylark/drake_cc.bzl line 888 at r5 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Missing new_linkopts
Done.
There was a problem hiding this comment.
Closes: #21836.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", missing label for release notes
rpoyner-tri
force-pushed
the
mold-integration
branch
from
544e206 to
08f8183
Compare
rpoyner-tri
force-pushed
the
mold-integration
branch
from
08f8183 to
cbc51eb
Compare
There was a problem hiding this comment.
Reviewed 2 of 4 files at r6, 3 of 3 files at r7, all commit messages.
Reviewable status: 2 unresolved discussions, needs at least two assigned reviewers, labeled "do not merge", missing label for release notes (waiting on @rpoyner-tri)
a discussion (no related file):
Working
Test locally.
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee sammy-tri(platform), labeled "do not merge", missing label for release notes
a discussion (no related file):
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
@drake-jenkins-bot linux-noble-unprovisioned-clang-bazel-experimental-debug please.
@drake-jenkins-bot linux-jammy-unprovisioned-gcc-bazel-experimental-debug please.
+@sammy-tri for platform review, please. Note that the DNM is because a new spin of Noble provisioned builds will be required.
There was a problem hiding this comment.
+@sammy-tri for platform review, please. Note that the DNM is because a new spin of Noble provisioned builds will be required.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee sammy-tri(platform), labeled "do not merge", missing label for release notes
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
@drake-jenkins-bot linux-noble-unprovisioned-clang-bazel-experimental-debug please. |
There was a problem hiding this comment.
Reviewed 1 of 4 files at r1, 1 of 4 files at r2, 2 of 4 files at r6, 3 of 3 files at r7, all commit messages.
Reviewable status: 2 unresolved discussions, labeled "do not merge", missing label for release notes (waiting on @rpoyner-tri)
rpoyner-tri
added
the
release notes: fix
There was a problem hiding this comment.
+(release notes: fix)
Reviewable status: 1 unresolved discussion, labeled "do not merge"
There was a problem hiding this comment.
Reviewable status: labeled "do not merge" (waiting on @rpoyner-tri)
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Working
Test locally.
LGTM
I tested gdb on clarabel_solver_test as well as the gdb command from the original bug report:
(gdb) b inflate_mesh_test.cc:91
Breakpoint 1 at 0xe7df14: inflate_mesh_test.cc:91. (5 locations)
(gdb) r
...
Breakpoint 1.2, __static_initialization_and_destruction_0 ()
at geometry/proximity/test/inflate_mesh_test.cc:92
92 GTEST_TEST(MakeInflatedMesh, NonConvexMesh) {
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, labeled "do not merge" (waiting on @rpoyner-tri)
a discussion (no related file):
Working
Think about impact on split dwarf and disk space?
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, labeled "do not merge" (waiting on @rpoyner-tri)
a discussion (no related file):
@rpoyner-tri to file an issue requesting new AMI deployment. (I'm promoting the "do not merge" label into a reminder discussion.)
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, labeled "do not merge"
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
@rpoyner-tri to file an issue requesting new AMI deployment. (I'm promoting the "do not merge" label into a reminder discussion.)
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, labeled "do not merge" (waiting on @rpoyner-tri)
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Working
Think about impact on split dwarf and disk space?
All good. I ran a local build of this PR and observed that (1) we still have split-dwarf dwo files appearing on disk, and (2) --fission=(yes|no) still has an effect on binary program size (much smaller with fission).
I am not sure why disk space limits might have been hitting on Jammy, but anyway Noble seems fine.
|
@drake-jenkins-bot retest this please |
|
@drake-jenkins-bot linux-noble-clang-bazel-experimental-everything-release please |
|
@drake-jenkins-bot linux-noble-clang-bazel-experimental-address-sanitizer please |
There was a problem hiding this comment.
Dismissed @rpoyner-tri from a discussion.
Reviewable status: labeled "do not merge" (waiting on @rpoyner-tri)
|
@drake-jenkins-bot linux-noble-gcc-bazel-experimental-debug please The The valgrind failure was just a timeout while running the test, we can ignore that for now. |
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, labeled "do not merge" (waiting on @rpoyner-tri)
a discussion (no related file):
Per f2f, the linux-noble-gcc-bazel-experimental-debug failure is probably real, and needs further attention.
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: 1 unresolved discussion, labeled "do not merge"
Closes: #21836.
This change is