[MSP430] Do not generate '@' character in symbol names. - #38286
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @arielb1 (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
cc @eddyb @michaelwoerister you may have some thoughts about this. I don't know if having a single target that uses a different format for symbol names would cause problems in debuginfo or incr. compilation or somewhere else. This target, msp430, is a little weird though because llvm can't generate object files for it so we are making llvm emit assembly (.s files) and then converting those into object files using gcc (msp430-gcc). So ... I think that means we can't even embed debuginfo into the object file in the first place (?). |
There was a problem hiding this comment.
Where does the @ even come from? prefix?
There was a problem hiding this comment.
No, prefix is ok, the @ comes from base_n
There was a problem hiding this comment.
The other solution would be to use smaller MAX_BASE on msp430. That way there will be no string allocation and copying.
There was a problem hiding this comment.
Aaaah. What is even MAX_BASE? FWIW @ is one of the characters we "escape" in symbols, on all targets.
So we don't actually consider it safe enough to use, although here it doesn't matter as much because it's unexported.
Another question nagging me is: do we really need this function? LLVM can handle multiple definitions with no name or with the same "informative" name without us needing to keep our own counters. Where are we keeping these Strings generated here, and if they're stored in CrateContext, why isn't the ValueRef kept instead?
There was a problem hiding this comment.
The function could probably be refactored away, yes.
As to MAX_BASE and @: Using a base64 encoding for the counter was an attempt to keep IR small after people complaining that it was too big. An unnecessary optimization as it turned out because it made no real difference.
michaelwoerister
left a comment
michaelwoerister
left a comment
There was a problem hiding this comment.
Thanks for the PR!
Since the counter containing the @ is something purely cosmetic this could also easily be handled consistently for all platforms. My suggestion would be:
- Add a
pub const ALPHA_NUMERIC_ONLY = 62tobase_n... - ... and use that instead of
MAX_BASEingenerate_local_symbol_name.
@pftbest, would you mind adapting the PR in this way?
There was a problem hiding this comment.
The function could probably be refactored away, yes.
As to MAX_BASE and @: Using a base64 encoding for the counter was an attempt to keep IR small after people complaining that it was too big. An unnecessary optimization as it turned out because it made no real difference.
MSP430 assembler does not like '@' character in symbol names, so we should only use alphanumerics when we generate a new name. Fixes rust-lang#38116
908bbf8 to
9d764fc
Compare
|
@michaelwoerister Updated the PR |
|
📌 Commit 9d764fc has been approved by |
[MSP430] Do not generate '@' character in symbol names. MSP430 assembler does not like '@' character in symbol names, so we need to replace it with some other character. Fixes #38116
8 participants
MSP430 assembler does not like '@' character in symbol names, so we need
to replace it with some other character.
Fixes #38116