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

DSPTool: Fix missing error when redefining labels #11541

Merged

Conversation

Pokechu22
Copy link
Contributor

The logging was broken in 958cbf3 (DSPTool doesn't use dolphin's logging system, so it just produced nothing; the same thing affected comparing before 693a29f).

AssemblerError::LabelAlreadyExists (previously ERR_LABEL_EXISTS) simply was never used.

The logging was broken in 958cbf3 (DSPTool doesn't use dolphin's logging system, so it just produced nothing; the same thing affected comparing before 693a29f).

AssemblerError::LabelAlreadyExists (previously ERR_LABEL_EXISTS) simply was never used.
@Pokechu22
Copy link
Contributor Author

For context, I created a loop with a label wait_dma_finish in one of my tests, but it turns out that label is also used by the included dsp_base_noirq. This produced an infinite loop where stuff was pushed to the stack each time, eventually leading to a stack overflow/exception 1. But, that only happens if it needs to loop, which wasn't the case on dolphin since DMAs complete instantly, so it only crashed like that on real hardware.

{
WARN_LOG_FMT(AUDIO, "Redefined label {} to {:04x} - old value {:04x}\n", label, lval,
*old_value);
DeleteLabel(label);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not familiar with this code, this isn't needed anymore because we early out and don't add the new label?

Copy link
Contributor Author

@Pokechu22 Pokechu22 Feb 5, 2023

Choose a reason for hiding this comment

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

Yeah, I changed the behavior from replacing the old label with a new one to not replacing the label.

I also think the behavior was buggy if the old label value matched the new one, as then there would be 2 labels in the list with the same value.

DSPTool has an option to continue assembling even if errors occur, so we do need to choose a behavior.

Copy link
Contributor

@iwubcode iwubcode left a comment

Choose a reason for hiding this comment

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

Untested but LGTM

@AdmiralCurtiss AdmiralCurtiss merged commit 9b5c52a into dolphin-emu:master Feb 10, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants