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: Add the name as-written, separate from the mangled name #4253

Merged
merged 8 commits into from
Aug 28, 2024

Conversation

dwblaikie
Copy link
Contributor

There's not much mangling happening yet - but Run -> main (and some overloading numbering happening, maybe LLVM is doing that 'helpfully' under the hood?) is enough to demonstrate this improvement/fix.

Ah, here it is:

#0  llvm::ValueSymbolTable::makeUniqueName (this=0x50287fe5b6c0, V=0x50287fe827e8, UniqueName="F") at external/_main~llvm_project~llvm-project/llvm/lib/IR/ValueSymbolTable.cpp:45
#1  0x000055555c40f964 in llvm::ValueSymbolTable::reinsertValue (this=0x50287fe5b6c0, V=0x50287fe827e8) at external/_main~llvm_project~llvm-project/llvm/lib/IR/ValueSymbolTable.cpp:100
#2  0x000055555c2a91df in llvm::SymbolTableListTraits<llvm::Function>::addNodeToList (this=0x50287fd16f18, V=0x50287fe827e8) at external/_main~llvm_project~llvm-project/llvm/lib/IR/SymbolTableListTraitsImpl.h:75
#3  0x000055555c2a90e5 in llvm::iplist_impl<llvm::simple_ilist<llvm::Function>, llvm::SymbolTableListTraits<llvm::Function> >::insert (this=0x50287fd16f18, where=..., New=0x50287fe827e8)
    at external/_main~llvm_project~llvm-project/llvm/include/llvm/ADT/ilist.h:166
#4  0x000055555c27fef2 in llvm::iplist_impl<llvm::simple_ilist<llvm::Function>, llvm::SymbolTableListTraits<llvm::Function> >::push_back (this=0x50287fd16f18, val=0x50287fe827e8) at external/_main~llvm_project~llvm-project/llvm/include/llvm/ADT/ilist.h:250
#5  0x000055555c27faeb in llvm::Function::Function (this=0x50287fe827e8, Ty=0x50287fd43058, Linkage=llvm::GlobalValue::ExternalLinkage, AddrSpace=0, name="F", ParentModule=0x50287fd16f00) at external/_main~llvm_project~llvm-project/llvm/lib/IR/Function.cpp:521
#6  0x0000555559441f95 in llvm::Function::Create (Ty=0x50287fd43058, Linkage=llvm::GlobalValue::ExternalLinkage, AddrSpace=0, N="F", M=0x50287fd16f00) at external/_main~llvm_project~llvm-project/llvm/include/llvm/IR/Function.h:175
#7  0x000055555c27ebac in llvm::Function::Create (Ty=0x50287fd43058, Linkage=llvm::GlobalValue::ExternalLinkage, N="F", M=...) at external/_main~llvm_project~llvm-project/llvm/lib/IR/Function.cpp:398
#8  0x0000555558ce7bb5 in Carbon::Lower::FileContext::BuildFunctionDecl (this=0x7fffffffc438, function_id=...) at toolchain/lower/file_context.cpp:257

That's where LLVM decides to make a new name (name.number) when asked to create a new global with the same name as an existing global.

It's not a valid mangling scheme - since the name won't be stable between different compilations, but it is enough to make single-compilation code build/run for now.

@dwblaikie
Copy link
Contributor Author

Hold off on this got this a bit messed up with some mangler prototyping leaking in here - don't mind me while I juggle git some more...

Copy link
Contributor

@jonmeow jonmeow left a comment

Choose a reason for hiding this comment

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

Thanks! Sorry this took a bit to work through.

Note the mangler files are still in the PR, so you'll get one more comment: opt_name -> name? (I assume you were doing opt_name due to the StringRef name, but in this PR it contrasts with name used for the other GetAsStringIfIdentifier)

There's not much mangling happening yet - but Run -> main (and some
overloading numbering happening, maybe LLVM is doing that 'helpfully'
under the hood?) is enough to demonstrate this improvement/fix.

Ah, here it is:
    at external/_main~llvm_project~llvm-project/llvm/include/llvm/ADT/ilist.h:166

That's where LLVM decides to make a new name (name.number) when asked to
create a new global with the same name as an existing global.

It's not a valid mangling scheme - since the name won't be stable
between different compilations, but it is enough to make
single-compilation code build/run for now.
@dwblaikie
Copy link
Contributor Author

Thanks! Sorry this took a bit to work through.

All good - appreciate your patience.

Note the mangler files are still in the PR,

Oh, yeah, wrangling git... OK, force push to remove the commit that added that in.

so you'll get one more comment: opt_name -> name? (I assume you were doing opt_name due to the StringRef name, but in this PR it contrasts with name used for the other GetAsStringIfIdentifier)

Done.

Copy link
Contributor

@jonmeow jonmeow left a comment

Choose a reason for hiding this comment

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

Thanks! FYI, I think this lost the semi-related change to CHECK in the other name handling, but since you're looking at name mangling already I think it makes sense to just move forward here and deal with that separately.

@jonmeow jonmeow enabled auto-merge August 27, 2024 23:03
auto-merge was automatically disabled August 28, 2024 18:46

Head branch was pushed to by a user without write access

@jonmeow jonmeow enabled auto-merge August 28, 2024 19:10
@jonmeow jonmeow added this pull request to the merge queue Aug 28, 2024
Merged via the queue into carbon-language:trunk with commit 67b287a Aug 28, 2024
8 checks passed
@dwblaikie dwblaikie deleted the mangle branch August 28, 2024 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants