Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(debuginfo): Add support for debuginfo, without scope support #455
Changes from 6 commits
c638def
2ffe9d1
8879155
5b053a3
6170f48
09fd908
51cd5f1
eaeb544
ef158f2
e18d3c3
fba0dae
8c975d9
9cc0a42
7c3565e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Large diffs are not rendered by default.
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.
Please use the exact same code as in the LLVM codegen since this will make it easier for people that wants to make changes in both versions.
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.
Well, the code is actually not completely portable. Since the scope part is not currently supported(and I haven't found the corresponding structure & interface for a scope in GCC), I have to currently separate the code from the original LLVM code. I never made it to a correct scope implementation. This will be fixed when the scope support is ready, or maybe never, if it is proved unnecessary.
Maybe you can also hint me in the certain interface of (debug)scope in GCC. I will handle it once I have time.
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.
In ef158f2, I added the original code as a comment as it is currently not in use for an empty implementation of
DIScope
.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.
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 thematch
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?
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.
Short answer: It's not that different. I will switch back to the original implementation.
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.
I originally wrote this stuff by hand and end up noticing the function being reusable. So I switched to their implementation. I choose a tuple in place of a struct because it seems to me that it is easier to be done not defining a data structure for something that is used only a few times internally.
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.
Yeah, I plan to attempt to deduplicate some code between cg_gcc and cg_llvm in the future. Not sure if this specific function can be deduplicated, but I guess a couple a other functions are.
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.
Maybe this should be a good idea to do it upstream, but it also depends on the attitude of developers of other backends such as those of cranelift.
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.
Yes, it's the plan. Cranelift doesn't use the SSA traits as far as I know.