Skip to content
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

Dbg with inspect #6116

Merged
merged 10 commits into from
Nov 30, 2023
Merged

Dbg with inspect #6116

merged 10 commits into from
Nov 30, 2023

Conversation

bhansconnect
Copy link
Contributor

@bhansconnect bhansconnect commented Nov 30, 2023

This could be submitted now, but there are still a number of nice to have changes left.

At a minimum, all platforms should be updated to include roc_dbg before this is submitted.
Unless I missed one, all platforms should have roc_dbg now.

Other changes that would be nice to have as well:

  • Wire file path to dbg so that we can make the location be file/path:line_number. (we could maybe even look at adding a richer context like rusts dbg! that shows a code snippet.
  • Generate an impl of roc_dbg for llvm and dev backend to use when running tests and/or repl.
  • Setup roc_dbg for the web repl.
  • Change code gen such that roc_dbg is created during normal builds. We no longer need to limit roc_dbg to just the dev command. I think it should be added for build and probably also run commands.
  • Update all uses for roc_panic in platforms to correctly take a *RocStr instead of a *char.

crates/compiler/builtins/bitcode/src/utils.zig Outdated Show resolved Hide resolved
@@ -996,7 +996,7 @@ fn fix_values_captured_in_closure_expr(
..
}
| Dbg {
loc_condition,
loc_message: loc_condition,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider doing the local rename here too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I renamed it to loc_condition so that I wouldn't need a separate match statement. This enables it to be match with Expect which still uses the name loc_condition

region: loc_expr.region,
})
}
LowLevelDbg(_, _) => unreachable!("Only exists after desugaring"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this a call to a low-level instead of introducing a new variant in this enum?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe. I think that would require resolving the other function argument at this point. Currently I have only resolved the actually debug string. I am missing the location (should be file path and line number in the long term). If I can figure out a way to get/pipe that information here, I think it can just be a low level (which I think would be a lot nicer).

crates/compiler/can/src/operator.rs Show resolved Hide resolved
crates/compiler/fmt/src/expr.rs Outdated Show resolved Hide resolved
crates/compiler/fmt/src/spaces.rs Outdated Show resolved Hide resolved
crates/compiler/gen_llvm/src/llvm/build.rs Outdated Show resolved Hide resolved
ostcar added a commit to ostcar/aoc2023 that referenced this pull request Nov 30, 2023
ostcar added a commit to ostcar/aoc2023 that referenced this pull request Nov 30, 2023
@ostcar
Copy link
Contributor

ostcar commented Nov 30, 2023

I have tested this with with my AoC platform, and it works perfectly fine. I hope, you can merge this soon.

Currently, the roc platform has to export the symbols roc_getppid, roc_mmap and roc_shm_open. I though, there were needed for the old debugging system. But when I use this PR, there are still required by roc.

Is it possible to remove the requirement?

@bhansconnect
Copy link
Contributor Author

bhansconnect commented Nov 30, 2023

Is it possible to remove the requirement?

They are also used for roc test and is what listens for the results of expect. That said, tests don't connect to platforms except for the compiler itself (IIUC). So I think all platforms except for the compiler should be able to remove it. Not fully sure of the scope of that, but I guess it should be added as another thing to re-evaluate.

@folkertdev. any thoughts on this? Can platforms remove roc_shm_open and friends?

Long term I assume we may want to simplify how we generate expects such that the compiler essentially is a special testing platform that expects directly hook into, but maybe that is wrong. Just know that the IPC has been flaky at times and also only works for unix (though maybe the flakiness was only a dbg issue and not a problem with expect?)

@bhansconnect bhansconnect merged commit 02d97bc into main Nov 30, 2023
16 checks passed
@bhansconnect bhansconnect deleted the dbg-with-inspect branch November 30, 2023 20:38
@rtfeldman
Copy link
Contributor

Thanks so much @bhansconnect!!! 🤩 🤩 🤩

I know there's some feedback here, but it doesn't look like any of it is blocking, and with Advent of Code starting tomorrow, I really want to get this in so people can have an improved dbg experience!

@bhansconnect
Copy link
Contributor Author

Yeah, filed an issue on the follow up changes that are still needed: #6119

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants