-
Notifications
You must be signed in to change notification settings - Fork 303
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
Enable for llvm-symbolizer for GitHub test workflow #1254
Conversation
3e06b9b
to
f10db64
Compare
Any chance this could get reviewed + merged this week? I've got a PR the DO team would really like to get out but have a segfault in mac CI; this would be very helpful! :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two concerns:
- The Linux version of the sed command is not having an effect for me locally on Ubuntu, removing the
=
fixes it for me. The mac version works atm. - Note that we continue to use the macOS-12 runner image, which has Xcode 14/clang 14, so the comment should be updated. With
brew --prefix llvm@15
, this will break when the runner image gets updated, let's just usebrew --prefix llvm
instead. This assumes having a potentially newer symbolizer version than the version workerd is compiled with is not going to cause problems.
f10db64
to
c33b0e1
Compare
Oops, sorry, fixed.
My devbox mac has brew installed llvm@14 and llvm@15, like the runner, and One resolution to this would be to add a wrapper script that does this on the fly for both Linux and Mac. Versions could roll forward at any time. |
For me, /opt/homebrew/opt/llvm is symlinked to the latest installed version of homebrew llvm, so this should still work, I can run |
c33b0e1
to
7ac193b
Compare
There is no symlink here, so I'm not sure that I'd want to rely on it. Pushed a minor revision to see what the builder has. This is not the most expedient path though. |
7ac193b
to
6ccbe04
Compare
From our macOS runner (Intel rather than Apple silicon):
|
During setup put the appropriate path for llvm-symbolizer into `.bazelrc` for Linux and macOS. Fix: #1247
6ccbe04
to
c9f8cd3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, in that case going with llvm@15
should work for now.
Thanks Felix, much appreciated. |
Thank you! |
Hopefully this helps pinpoint the issue from CI. If it could be helpful to pair for a while with some with a macOS machine and |
Looks like the stack is empty on mac 🙁
Before pairing it might be quicker to have someone with a mac to run the test ( |
I get the exact same output with the empty stack trace on mac locally. @ohodson is it possible to run |
Not to stray too far from this PR but if anyone has a windows machine and can run it that might work too? The same test fails on Windows due to access violation. |
FWIW, we have most of the *.wdtests available in vscode already for debugging, selecting the 'workerd wdtest test case (dbg)'. At the time of writing, we don't have those that need custom info, like |
@MellowYarker , a workerd ASAN build and test on Linux on the
|
The asan build on Linux is probably easiest to repro and investigate. For posterity, here's the stack from Windows:
|
During setup put the appropriate path for llvm-symbolizer into
.bazelrc
for Linux and macOS.Bug: #1247