Skip to content

alloca seems buggy on M4 #548

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

Merged
merged 2 commits into from
Jan 24, 2018

Conversation

dhalbert
Copy link
Collaborator

Fixes #521. The problem appears to be bad compilation of alloca(). alloca() can be replaced with a run-time variable array: that works.

I replaced a few other uses of alloca() that were similar, just in case they might also be problematic. There are some uses where alloca() is not done at the point of declaration, and those are harder to change. Also there are submodules with lots of uses of alloca() that I didn't change.

Eventually I will try to come up with a small test case and submit a gcc bug report.

@dhalbert dhalbert requested a review from tannewt January 24, 2018 01:58
tannewt
tannewt previously approved these changes Jan 24, 2018
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Great find!

@jerryneedell
Copy link
Collaborator

Great job! - I just tried this PR on my M4 and it works fine in executing the same code that failed for #521

@dhalbert
Copy link
Collaborator Author

dhalbert commented Jan 24, 2018

Unix builds are failing import tests! I'll look at it.

@tannewt
Copy link
Member

tannewt commented Jan 24, 2018 via email

@dhalbert
Copy link
Collaborator Author

Now travis is happy. I had to remove a different alloca() that I had changed to an array. It went out of scope and was destroyed, but the data was needed later. alloca() storage lives until the function returns, so that was wrong in this case.

Original alloca() changed to an array still stands, to fix the bug.

@jerryneedell
Copy link
Collaborator

I repeated the tests from #521 and this PR still works fine. Thanks.
Do yo have any idea why this cropped up when it did. Was there some change that triggered it or was it just a latent issue and we got lucky?

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Ok, cool! Should we try to remove it in the future?

@tannewt tannewt merged commit a5bfbbc into adafruit:master Jan 24, 2018
@dhalbert
Copy link
Collaborator Author

I did some further testing on this and found that the problem arose when the double-tap stuff was added to the loader map. There's no bug with this:

/* top end of the stack */
_estack = ORIGIN(RAM) + LENGTH(RAM);

The bug appears when _estack is moved down one word to make room for the double-tap check word:

/* top end of the stack */
_estack = ORIGIN(RAM) + LENGTH(RAM) - 4;
_bootloader_dbl_tap = _estack;

I actually just adjusted _estack by -4 and it caused the problem, even without introducing the rest of the double-tap code.

Then I changed it to -8 to make it double-word aligned and the bug disappears again.

_estack = ORIGIN(RAM) + LENGTH(RAM) - 8;

@tannewt: I'll check in with you about this.

@jerryneedell
Copy link
Collaborator

So - does this imply that there is really no issue with "alloca"? It is just a memory alignment issue?

@dhalbert
Copy link
Collaborator Author

@jerryneedell I don't know yet. I have something else to do for a while and then will come back to this. It could be that alloca() assumes certain alignment, which isn't true, and the array decl doesn't make that assumption. The generated code does not differ between the working and non-working versions.

@tannewt
Copy link
Member

tannewt commented Jan 24, 2018 via email

@dhalbert dhalbert deleted the 3.0_alloca_problem_issue_521 branch January 25, 2018 03:12
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