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

feat(debuginfo): Add support for debuginfo, without scope support #455

Merged
merged 14 commits into from Feb 28, 2024

Conversation

tempdragon
Copy link
Contributor

This PR depends on rust-lang/gccjit.rs#28.

TODO:
1. Add int.rs locations
2. Add demangling support
3. Add debug scope support
4. Add vtable support
5. Clean up builder.rs locations
TODO:
1. Clean the unnecessary locations in builder.rs & int.rs
2. Add demangling support
3. Add debug scope support
4. Add vtable support
5. Clean up builder.rs locations
@tempdragon
Copy link
Contributor Author

tempdragon commented Feb 24, 2024

TODO:

  1. Clean up the unnecessary locations in builder.rs & int.rs
  2. Add demangling support
  3. Add debug scope support
  4. Add vtable support
  5. Clean up builder.rs locations

@antoyo
Copy link
Contributor

antoyo commented Feb 25, 2024

Thank you so much! I can't wait to try this.

I'm perfectly fine with merging this PR as is, but I was wondering if you knew why rustc doesn't handle the debuginfo in the builder.rs module directly like is done here.
The idea I thought was maybe for performance reasons, since you can disable emitting debug info and might not want to create the objects for locations and names in this case.
Do you have any ideas?

@tempdragon
Copy link
Contributor Author

tempdragon commented Feb 25, 2024

Short Answer

I guess I knew it. In fact, it was my initial idea of applying the debuginfo.
Currently, this implementation shouldn't create too much overhead but a few loads of the None from self.loc, if the self.sess().opts.debuginfo is false.

Warning

This version of debuginfo emission looks miserable now, don't expect too much, and this is only a proof of concept/"draft" at this time.

Experiment

This experiment is to show that no GCC debug location is to be generated, if self.sess().opts.debuginfo == false.

You can simply put some println! in the DebugInfoMethods<'tcx>::dbg_loc, and then make create_function_debug_context an empty impl. returning only None and finally disable the debug flag when compiling tests and examples using cg_gcc(making self.sess().opts.debuginfo == false ), after which you will notice that no println! is called, indicating no debuginfo is generated.

My Guesses

Rustc relies on the per-function FunctionCx.debug_context to store the location & variable debug information per function. A None in this field will be passed through FunctionCx::adjusted_span_and_dbg_scope() to FunctionCx::dbg_loc() in a predicate and prevent it from calling self.cx.dbg_loc(dbg_scope, inlined_at, span) to generate a Location.

Since the generation of function debug context is banned on the first line by the create_function_debug_context in its first few lines, the generation of Location is therefore impossible.

It seems, though I have no evidence to this, that an implementation with debuginfo applied to the target afterwards relies on the scope to be implemented. Even if that method is our final destination(And I do believe it will be, as long as the cg_gcc can last longer and better than gccrs.), we are still in need of more modifications to the gccjit. I do have a private fork of that gccjit version in my computer, but having discovered that debuginfo generation can be accomplished without too much modification to gccjit, using the combination you see, I decided to create such an experimental version of debug info.

Further Work

Whether we should use the update approach or the non-standalone approach, we will always need the scope support. Currently, this is scheduled after the set_name. To optimize out the loads, we will have to wait for the scope implementation to complete.

@tempdragon
Copy link
Contributor Author

However, I do mean to have it tested using the CI since there are always suprises.

@tempdragon
Copy link
Contributor Author

To set the debuginfo afterwards, we first have to read the mapping from the debuginfo to the object from a separate function, this has a similar overhead to doing it in builder.rs.
Since location creation is disabled through scope generation in both cg_llvm and cg_gcc, no extra overhead is needed.

src/builder.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@antoyo antoyo left a comment

Choose a reason for hiding this comment

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

Sending what I reviewed for now.

src/base.rs Outdated Show resolved Hide resolved
src/base.rs Outdated Show resolved Hide resolved
src/builder.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@antoyo antoyo left a comment

Choose a reason for hiding this comment

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

Awesome work! (Sorry for all the nitpicks.)

src/builder.rs Outdated Show resolved Hide resolved
src/builder.rs Outdated Show resolved Hide resolved
src/builder.rs Outdated Show resolved Hide resolved
src/builder.rs Outdated Show resolved Hide resolved
src/builder.rs Outdated Show resolved Hide resolved
src/debuginfo.rs Outdated Show resolved Hide resolved
src/debuginfo.rs Outdated Show resolved Hide resolved
src/debuginfo.rs Outdated Show resolved Hide resolved
src/debuginfo.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@antoyo antoyo left a comment

Choose a reason for hiding this comment

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

Some more nits.

src/builder.rs Outdated Show resolved Hide resolved
src/builder.rs Outdated Show resolved Hide resolved
src/debuginfo.rs Outdated Show resolved Hide resolved
src/debuginfo.rs Outdated
Comment on lines 160 to 163
if ! self.sess().target.is_like_msvc {
col } else {
UNKNOWN_COLUMN_NUMBER
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand the difference compared with the code from cg_llvm. It just seems to me that the condition is_like_msvc was moved from after the match to inside it.

And that the return type was changed from a struct to a tuple containing the same information. Could you please explain to me how this is different?

1. Revert to the original `lookup_debug_loc` of DebugLoc return type
2. Removed the commented code of scope lookup
@antoyo antoyo merged commit 6ec5010 into rust-lang:master Feb 28, 2024
34 checks passed
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.

None yet

2 participants