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.

Sign up for GitHub

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

Jump to bottom

ASSERT in every app on ARM: opnd_t size changed #4520

Closed
derekbruening opened this issue Nov 4, 2020 · 7 comments · Fixed by #4521
Closed

ASSERT in every app on ARM: opnd_t size changed #4520

derekbruening opened this issue Nov 4, 2020 · 7 comments · Fixed by #4521
Assignees
@derekbruening
Labels
Bug-Assert OpSys-ARM

Comments

@derekbruening
Copy link
Contributor

I assume this is a regression from PR #4467 3a408a0. Lack of ARM CI tests is really an issue.

95: <Application /home/derek/dr/git/build_dbg_tests/suite/tests/bin/client.signal (16492).  Internal Error: DynamoRIO debug check failure: /home/derek/dr/git/src/core/arch/arch.c:668 sizeof(opnd_t) == EXPECTED_SIZEOF_OPND
@derekbruening derekbruening self-assigned this Nov 4, 2020
derekbruening added a commit that referenced this issue Nov 5, 2020
@derekbruening
i#4520: Restore ARM opnd_t layout
510c7b2 
Avoids padding due to 8-byte-alignment of the new opnd_t.immed_double
field, to keep the layout how it was before PR #4467, avoiding an
init-time assert and problems matching the "black box" exported opnd_t
struct.

Fixes #4520
@derekbruening derekbruening mentioned this issue Nov 5, 2020
Merged
@AssadHashmi
Copy link
Contributor

Lack of ARM CI tests is really an issue.

It is. I'm in the process of acquiring AArch32 instances similar to the packet.net machines we already have for AArch64 pre-commit and development.

@AssadHashmi
Copy link
Contributor

Which ARM test(s) exposed this?

@derekbruening
Copy link
Contributor Author

#4522 is another regression on ARM not caught at commit time.

@derekbruening
Copy link
Contributor Author

Which ARM test(s) exposed this?

All of them: if the compiler lays out opnd_t to not match the "black box" binary interface version, every test will fail at init time.

I'm wondering why x86 32-bit Linux doesn't fail the same way: I would expect the compiler to pad the double to align it to 8 in the same way.

@derekbruening
Copy link
Contributor Author

I'm in the process of acquiring AArch32 instances similar to the packet.net machines we already have for AArch64 pre-commit and development.

Awesome! Would we be able to set up Jenkins on them?

@AssadHashmi
Copy link
Contributor

Awesome! Would we be able to set up Jenkins on them?

Depends what h/w we end up getting. If it's enough to run Jenkins, we'll do it.

@derekbruening
Copy link
Contributor Author

This issue covers just fixing the immediate ARM breakage. #4488 covers potentially changing the opnd_t layout to get the double field properly aligned to 8.

derekbruening added a commit that referenced this issue Nov 5, 2020
@derekbruening
i#4520: Restore ARM opnd_t layout (#4521)
2c67857 
Avoids padding due to 8-byte-alignment of the new opnd_t.immed_double
field, to keep the layout how it was before PR #4467, avoiding an
init-time assert and problems matching the "black box" exported opnd_t
struct.  We may later decide to align it and break compatibility: that's #4488.
Here we fix the immediate ARM breakage.

Issue: #4488, #4520
Fixes #4520
gregcawthorne pushed a commit that referenced this issue Nov 28, 2020
@derekbruening @gregcawthorne
i#4520: Restore ARM opnd_t layout (#4521)
9f95c77 
Avoids padding due to 8-byte-alignment of the new opnd_t.immed_double
field, to keep the layout how it was before PR #4467, avoiding an
init-time assert and problems matching the "black box" exported opnd_t
struct.  We may later decide to align it and break compatibility: that's #4488.
Here we fix the immediate ARM breakage.

Issue: #4488, #4520
Fixes #4520
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@derekbruening derekbruening

Labels
Bug-Assert OpSys-ARM
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

i#4520: Restore ARM opnd_t layout DynamoRIO/dynamorio
2 participants
@derekbruening @AssadHashmi