Skip to content

Keep the git textAddress pointer always up-to-date when instructions. Fixes issue #120.#122

Open
ricferpas wants to merge 1 commit intorarsm:masterfrom
ricferpas:fix-120-duplicate-text
Open

Keep the git textAddress pointer always up-to-date when instructions. Fixes issue #120.#122
ricferpas wants to merge 1 commit intorarsm:masterfrom
ricferpas:fix-120-duplicate-text

Conversation

@ricferpas
Copy link

…ctions, both for basic and psudoinstructions.

Previously it was not always updated for basic instructions, leading to "Duplicate text segment address" error when a program last instruction was not a pseudoinstruction and StartAtMain was true.

…ctions, both for basic and psudoinstructions.

Previously it was not always updated for basic instructions, leading to "Duplicate text segment address" error when a program last instruction was not a pseudoinstruction and StartAtMain was true.
@ladyslipper-glade
Copy link

Would it make sense to include a test case for this?

@ricferpas
Copy link
Author

Yes, this should have a testcase.However, It is not clear to me how to add it.

In fact, most of the files in the current test directory would serve as a testcases for this bug (for example: every file that ends with an ecall instruction fails to assemble without this fix). The problem is that the test suite is executed with the “StartAtMain” option disabled, so the problem does not manifest.

The problem was introduced by PR 117, and I guess that it was not detected at the time because the tests for PR 117 always end with “ret” which is a psudoinstruction and worked correctly.

How would you add the test?One option would be to modify RarsTest.java to execute all the tests with the StartAtMain option enabled. Another idea would be to execute all the test both with the option enabled and disabled, and another option would be to modify RarsTest to execute a few selected tests with this option enabled.

@ladyslipper-glade
Copy link

Setting aside for the moment the issue of the test, I was studying the PR and have a question about it. First, let me apologize in advance if my question is simplistic. I'm not very experienced (a college student), and I'm relatively new to Java. Okay, now my question:

Should the changes you propose go a little higher up? Specifically, should they be inside the condition

if (Memory.inTextSegment(statement.getAddress()))...

As it is, your change will also execute when instructions are assembled into the data segment, and I feel like that would send the text address off into space. For example, after your change, won't the ecall below get the wrong address?

.text
.globl main
main:
    li a7, 10
.data
    nop
.text
    ecall

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.

2 participants