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

debuginfo: ensure named types are package prefixed #4812

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

flga
Copy link
Contributor

@flga flga commented Feb 8, 2025

This allows debug scripts/debuggers to do proper type lookups.
Lots of whitespace :(

@gingerBill
Copy link
Member

I think the reason this was not done originally is because you cannot refer to that type due to the .. It might have to be pkg::name instead to match C++.

@flga
Copy link
Contributor Author

flga commented Feb 12, 2025

@gingerBill types are already declared in pkg.type form, just not consistently. Hence the change, I bumped into this while inspecting enum array debug info.

I'm not sure under which contexts it wouldn't work, but at least both lldb and gdb don't complain (DWARF).

Just wanted to clarify that if we want to move away from it we need to also change current logic even if we drop this change.

Sidenote: Go goes as far as including the full package path. I pondered putting that forward but that seemed like a big change and it really only matters for programs with duplicate package names so I figured I'd keep it simple for now and focus on the inconsistency. Irrelevant, package names are enforced to be unique as pointed out by Lperlind.

@Lperlind
Copy link
Contributor

You can't have duplicate package names in Odin so that sidenote won't be a problem

@flga
Copy link
Contributor Author

flga commented Feb 12, 2025

Ah, you are right, I assumed that if they had different roots it would be fine. Thanks!

@laytan
Copy link
Collaborator

laytan commented Feb 12, 2025

Note that this function is also used for some error messages and probably other things.
(just a note, I don't know if we want that or not)

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