Adapting jsiek's executable semantics tooling for commit. #237
Conversation
There was a problem hiding this comment.
Awesome! But please add instructions for building and running with Bazel. A naïve attempt yields:
$ bazel test
Loading:
Loading: 0 packages loaded
DEBUG: Rule 'rules_foreign_cc' indicated that a canonical reproducible form can be obtained by modifying arguments sha256 = "1dbab600e91d3fe2c9ff1baaf2bec929e8ffbf7e81f6b501e52783c86614bf83"
DEBUG: Call stack for the definition of repository 'rules_foreign_cc' which is a http_archive (rule definition at /private/var/tmp/_bazel_dabrahams/309deff96dbfdff03fd99c86c609773c/external/bazel_tools/tools/build_defs/repo/http.bzl:292:16):
- /Users/dabrahams/src/carbon-semantics/WORKSPACE:11:1
ERROR: no such package '@llvm_bazel//': The repository's path is "third_party/llvm-bazel/llvm-bazel" (absolute: "/Users/dabrahams/src/carbon-semantics/third_party/llvm-bazel/llvm-bazel") but this directory does not exist.
ERROR: no such package '@llvm_bazel//': The repository's path is "third_party/llvm-bazel/llvm-bazel" (absolute: "/Users/dabrahams/src/carbon-semantics/third_party/llvm-bazel/llvm-bazel") but this directory does not exist.
INFO: Elapsed time: 0.102s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (0 packages loaded)
ERROR: Couldn't start the build. Unable to run tests
FAILED: Build did NOT complete successfully (0 packages loaded)
Note: I really am that naïve; i.e. it's not obvious to me what to do to make this work.
|
I'm discussing the build issues with dabrahams separately -- this was an issue with missing git submodules, but there's other issues. However, contribution tools docs are intended to address the underlying "get tools working" issue. |
|
I am still unable to get this to build on any platform, even after following all the contribution tools instructions |
|
@dabrahams Not sure if you missed my response over IM (I responded about 15 minutes before you commented here), but "bison: command not found" means you don't have bison installed. If you're not familiar with bison, see here. The dependency is noted in the PR description: "System-local versions of bison/flex are used". Regardless, please note I'm just assuming you're commenting on the same build error that you did over IM. When you run into errors, it would really be more constructive to provide the errors that you're running into. That would also give others a chance to comment and debug without a round of "what error are you seeing?" |
|
I am familiar with bison. The first lesson is that this reveals a bug in
the contribution_tools doc.
I don't think it's the same error.
I know it doesn't seem like it, but I really have taken bug reporting 101.
I am, however, wary of flooding group channels with error logs before I
know what the group culture is. I could have used a pastebin I suppose,
but I was actually at the end of a day where I had already been working too
long and really needed to disconnect, but wanted the group to know there's
a problem.
…On Fri, Feb 5, 2021 at 9:53 AM Jon Meow ***@***.***> wrote:
@dabrahams <https://github.com/dabrahams> Not sure if you missed my
response over IM (I responded about 15 minutes before you commented here),
but "bison: command not found" means you don't have bison installed. If
you're not familiar with bison, see here
<https://www.gnu.org/software/bison/>. The dependency is noted in the PR
description: "System-local versions of bison/flex are used".
Regardless, please note I'm just assuming you're commenting on the same
build error that you did over IM. When you run into errors, it would really
be more constructive to provide the errors that you're running into. That
would also give others a chance to comment and debug without a round of
"what error are you seeing?"
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#237 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAKYILKBQVUTNYVWM5QK23S5QWAXANCNFSM4WBHMMLQ>
.
--
-Dave
|
|
OK, thanks for the hints so far. On Linux, I got this to work: After installing flex and bison with apt. Don't try to install them with brew unless you want to wait "forever." On Mac, we have: |
|
Just a reminder, the C++ toolchain doesn't support Mac yet... |
Co-authored-by: Chandler Carruth <[email protected]>
|
FWIW, on Mac, with or without trunk merged into this, I still can't build, and I don't understand the meaning of the errors: flex and bison are installed, FWIW. |
|
@dabrahams It looks like you're getting a parse error indicating an old version bison. xcode installs bison 2.3 (from 2006); please make sure you are using a recent version. |
|
@jonmeow thanks for the hint; I'll look into it. I thought I installed bison with brew but maybe it's getting bypassed. FWIW, I successfully built on linux after merging trunk into this branch. |
|
Per brew output on mac you need to add bison and flex on your path because they're trying not to conflict with system binaries, e.g.: |
| copts = [ | ||
| "-Wno-unneeded-internal-declaration", | ||
| "-Wno-unused-function", | ||
| "-Wno-writable-strings", |
There was a problem hiding this comment.
| "-Wno-writable-strings", | |
| "-Wno-writable-strings", | |
| "-std=c++14", |
Without this change, uses of the "register" keyword in the bison output go from being warnings to being errors on MacOS.
With this change, and brew's bison first in the path, I can report success on MacOS!
There was a problem hiding this comment.
Per discussion on Discord, I'm wary of this fix because it feels like a broad stroke. I think it'd probably be better to investigate options further, but I'd rather not block commit on this as the current state works for Linux.
There was a problem hiding this comment.
Why not take the broad stroke now to unblock Mac users (including the original author of the code) and open an issue to find a better fix?
There was a problem hiding this comment.
To validate, I successfully built on mac with no changes. I do get a register-related build error if I use an older version of flex, the one included by xcode.
As I previously noted, a recent version of flex is required, just as with bison. Please ensure you're using a recent version.
I think we need to be really cautious about changing how builds work; it can become more difficult to fix later. While I think no change should be needed here, in the future, please provide build errors and rationale for changing code. Sometimes, by thinking these issues through, we can get to the root of the issue and arrive at a different solution.
There was a problem hiding this comment.
Thanks for doing the diligent work. It didn't occur to me that the old flex was the cause, but that explains everything.
|
@jonmeow Thank you for all your work getting this landed. It's going to make a huge difference going forward. |
* Add assertion to detect UB in interpreter.cpp This change, applied to 2111b85 ("Adapting jsiek's executable semantics tooling for commit. (#237)"), causes the if2.6c test to segfault. Becase the assertion fires only when `stmt == nullptr` and no code has permission to change `stmt` (it is `const`) before it is dereferenced in the `switch`, and nothiing in `PrintStatement` is supposed to exit the program, the assertion is a valid change that detects a bug. The crash was originally manifest in 0213c6d ("Executable Semantics: 1st-class stacks (#296)"). * Temporarily disable the if2 test pending #311 See #311 * [executable semantics] Record exit code on expected error. This will prevent a final segfault from sneaking by, detected as a passing test. A more principled follow-up commit would bottleneck detected error exit reporting and have it write something to std::cerr that can be recognized.
Notes versus what jsiek wrote:
- This adopts Bazel for building.
- System-local versions of bison/flex are used. I found https://github.com/jmillikin/rules_bison, but those print a lot of warnings (things like -Wsign-compare IIRC) which makes builds hard to read. Plus I think the underlying bison_cc_library rule didn't work, so this would really only get a hermetic bison/flex build (helpful, but didn't seem worth more time).
- I'm adding in a .bazeliskrc to push a somewhat more standard choice of bazel versions. I noticed I was getting unstable versions by default, possible Google-specific, but seemed good to include.
- The `-lpthread` kludge.
- Turn all of the examples into golden tests.
- Including adding a golden test rule.
- Fixed various style guide issues. For example:
- Fixing function names to be CamelCase instead of snake_case (https://google.github.io/styleguide/cppguide.html#Function_Names)
- Removed exception use (https://google.github.io/styleguide/cppguide.html#Exceptions)
- File name fixes (https://github.com/carbon-language/carbon-lang/blob/trunk/docs/project/cpp_style_guide.md#file-names)
- Dropped `using` of `std` names -- I believe this is preferred (maybe we should be explicit about this in the Carbon style guide)
- Switched `enum` uses to `enum class` for ease-of-identification.
- Spent some time breaking out files to hopefully be easier to read/edit pieces, and understand relations between structs.
- Added `code requires` to `syntax.ypp` to address include issues
Possibly other things -- but the fundamental structure is, I believe, unchanged. I put in the golden tests pretty early to ensure I wasn't mutating output/results.
* Add assertion to detect UB in interpreter.cpp This change, applied to 6e5070d ("Adapting jsiek's executable semantics tooling for commit. (#237)"), causes the if2.6c test to segfault. Becase the assertion fires only when `stmt == nullptr` and no code has permission to change `stmt` (it is `const`) before it is dereferenced in the `switch`, and nothiing in `PrintStatement` is supposed to exit the program, the assertion is a valid change that detects a bug. The crash was originally manifest in 29a5994 ("Executable Semantics: 1st-class stacks (#296)"). * Temporarily disable the if2 test pending #311 See #311 * [executable semantics] Record exit code on expected error. This will prevent a final segfault from sneaking by, detected as a passing test. A more principled follow-up commit would bottleneck detected error exit reporting and have it write something to std::cerr that can be recognized.
Notes versus what jsiek wrote:
-lpthreadkludge.usingofstdnames -- I believe this is preferred (maybe we should be explicit about this in the Carbon style guide)enumuses toenum classfor ease-of-identification.code requirestosyntax.yppto address include issuesPossibly other things -- but the fundamental structure is, I believe, unchanged. I put in the golden tests pretty early to ensure I wasn't mutating output/results.