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

Current "inline-everything" approach without context limits debugger support #1527

Closed
mitchmindtree opened this issue May 12, 2022 · 6 comments
Labels
compiler General compiler. Should eventually become more specific as the issue is triaged

Comments

@mitchmindtree
Copy link
Contributor

I just noticed this issue at the debugger repo from @Voxelot FuelLabs/fuel-debugger#3.

Opening an issue so we can track/discuss how we might better support debugging at a higher-level.

Perhaps we can provide some sort of mapping from regions of generated bytecode to their original Spans? I'm not yet familiar enough with codegen to know how this is handled in langs like Rust or C for tools like gdb etc.

@mitchmindtree mitchmindtree added the compiler General compiler. Should eventually become more specific as the issue is triaged label May 12, 2022
@otrho
Copy link
Contributor

otrho commented May 12, 2022

We can realistically turn off the inline-everything approach now with some work, it just hasn't seemed worth it quite yet, priority wise. Source mapping should just fall out of that. Some dot points:

  • The VM doesn't have an explicit intra-code calling mechanism like a CPU call/ret, we need to do it manually.
  • That is why it's easier for now to just inline everything, which:
    • Can be wasteful.
    • Disallows recursion.
  • We have always inlined everything in the AST.
  • When introducing IR we added the ability to have calls and use an inlining pass. It actually recreates the functions from the inlined versions in the AST before re-inlining them because it's marginally less complex that way.

I think if we stop inlining in the AST and preserve function source spans then it should filter through the IR and remain in generated bytecode. Our namespacing may need further work to ensure absolute symbols and zero clashes, not sure. We can still inline everything (with the pass) and not need to implement internal calls. This is not trivial but not a crazy amount of work, IMO.

OTOH, just beefing up the current AST with absolute symbols might just be enough to solve this specific problem.

@mohammadfawaz
Copy link
Contributor

In LLVM, debug information in IR contain a field called "inlined_at". The inliner optimization pass adds these, so we could do the same. (Inlining happens in IR for us as well)

@mohammadfawaz
Copy link
Contributor

mohammadfawaz commented May 12, 2022

@otrho, wait don't we in-line everything after IR gen now?

@otrho
Copy link
Contributor

otrho commented May 12, 2022

The problem is we still inline into the AST and throw away a bunch of metadata at that point, and so we don't have enough info for the source debugger.

The IR then takes those inlined function bodies and recreates the functions as IR functions. It then inlines them again with a pass. Doing it in two stages like that is slightly simpler, both in code and cognitively, even though it's a bit dumb. OTOH writing an inlining pass was good for testing, etc.

@adlerjohn
Copy link
Contributor

adlerjohn commented May 12, 2022

Would it be beneficial for the VM to support subroutines natively? Or would a relative-jump or jump table be sufficient?

@otrho
Copy link
Contributor

otrho commented May 13, 2022

We had that discussion a while ago, didn't we..? I think we decided that jump table idea was going to be OK, though off the top of my head I've forgotten how it would work again. 🙂

A bit like continuation passing style, except with tokens rather than actual addresses? I think that was it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler General compiler. Should eventually become more specific as the issue is triaged
Projects
Status: Done
Development

No branches or pull requests

5 participants