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

Preserve case when outputting symbols in sym format #155

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

Conversation

Zeturic
Copy link
Contributor

@Zeturic Zeturic commented Sep 1, 2019

Sorry for not opening an issue first. But basically:

The sym format normally lowercases all of the symbols when writing the sym file. This can be irritating when the "canonical" version of the name isn't the lowercased one - e.g. in C-land it's Foo but ARMIPS ultimately outputs foo in the symbol file, because No$GBA doesn't treat them case-insensitively and thus doesn't recognize that Foo exists.

This PR makes both sym case-preserving (sym2 already was), avoiding that issue.

If you'd rather this be combined into a single commit, just let me know.

Edit: I went ahead and squashed this into one commit.

@Zeturic
Copy link
Contributor Author

Zeturic commented Sep 1, 2019

Oh, whoops. I just realized that e595501 is buggy. If the capitalization doesn't match between the -definelabel on the command line and the usage in the file, ARMIPS doesn't recognize them as being the same label.

I've reverted it for now, while I work on having -definelabel labels properly be case-preserving.

@Zeturic
Copy link
Contributor Author

Zeturic commented Sep 18, 2019

I rebased against the most recent master (though admittedly there were no conflicts).

Also, I forgot to mention when I initially posted this, but if there's some compelling technical reason why -sym has to stay as it is (all lowercase), I could modify the case-preserving version to be -sym3 instead.

@sp1187 sp1187 requested a review from Kingcom September 18, 2019 06:36
@Kingcom
Copy link
Owner

Kingcom commented Oct 25, 2019

Sorry, I forgot about this one. The -sym option was meant to be compatible with the no$gba assembler, which is why it also lowercases everything. -sym2 was an extension made to support function sizes in PPSSPP, which does not lowercase the labels. If you do not use .function, the -sym2 output should be identical to the -sym output except for the label cases.

It is probably worth discussing if keeping the no$gba behavior is worth it. Its assembler was never particularly good, and I doubt there are many people who depend on this behavior.

@Zeturic
Copy link
Contributor Author

Zeturic commented Jan 26, 2020

I meant to respond to this much earlier, sorry.

The reason I couldn't personally use -sym2 was because mGBA doesn't understand the format including function sizes, and I couldn't use -sym because mGBA treats the labels case-sensitively so if you (e.g.) try to set a breakpoint for Foo it doesn't recognize it unless you change it to foo.

As a small aside, one of the changes I made in the PR was to preserve the originalName of a -definelabel label should probably be integrated regardless, because -definelabels were (as far as I know) the only time -sym2 doesn't properly preserve case as its supposed to. I can extract just that change into its own PR if preferred.

I'll also be honest, I'm not actually familiar with a standalone No$GBA assembler. I've only ever used these sorts of files as an input to the No$GBA debugging emulator (which yes, does allow you to assemble code at particular locations in a ROM while it's running, but I don't think that's what you were referring to), or more recently, doing the same thing in mGBA.

@sp1187
Copy link
Collaborator

sp1187 commented May 1, 2020

Do you think you could send the -definelabel case fix as a separate PR, as you suggested before?

@Zeturic
Copy link
Contributor Author

Zeturic commented Sep 17, 2020

Because it is at least possible that changing -sym's behavior would break someone's workflow, I implemented a -sym3 that behaves as a case-preserving -sym, as I had suggested before. You can look at the commit here (I didn't push it to this PR, yet).

It's small, doesn't affect -sym's behavior, and still solves my problem.

I can force push that commit into this PR branch if desired.

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.

3 participants