Skip to content

Fix big endian issues #1710

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

Closed
wants to merge 3 commits into from
Closed

Conversation

roehling
Copy link
Contributor

This PR fixes a few test failures if the host architecture is Big Endian:

  • On BE architectures, truncating an integer type will change its value, because the lower bits will be cut off, not the higher ones. Most notably, the program counter in uc_emu_start will end up zeroed. Luckily, the existing set_pc function handles all corner cases correctly.
  • Reading guest memory leaks the endianness to the host, so if the host has a different endianness, this needs to be compensated for. I added a bunch of cpu_to_le and le_to_cpu functions (which are no-ops on LE hosts) for this.

The endianness of the guest leaks to the host if memory is read. This
needs to be dealt with if the host endianness differs from the guest
endianness.
@roehling
Copy link
Contributor Author

After applying this PR, these two checks will continue to fail on BE hosts, and I am not entirely sure what the problem is. I disabled these two checks in Debian for now, but I hope someone can come up with an actual fix.

@wtdcode
Copy link
Member

wtdcode commented Sep 25, 2022

I failed to understand the context here, anyway I can reproduce this?

@roehling
Copy link
Contributor Author

The test failures happen if you run UC on a host with big endian architecture, such as PowerPC, S390X, etc.
For example, see https://buildd.debian.org/status/fetch.php?pkg=unicorn-engine&arch=s390x&ver=2.0.0-1&stamp=1662876884&raw=0

@@ -4,6 +4,7 @@
#include <stdio.h>
#include <stdint.h>
#include <unicorn/unicorn.h>
#include "qemu/bswap.h"
Copy link
Member

Choose a reason for hiding this comment

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

This is wrong since we don't import qemu things in test files as they are not linked against the test targets, though it may work...

@@ -1603,7 +1603,7 @@ static void gen_xxspltw(DisasContext *ctx)
tofs = vsr_full_offset(rt);
bofs = vsr_full_offset(rb);
bofs += uim << MO_32;
#ifndef HOST_WORDS_BIG_ENDIAN
#ifndef HOST_WORDS_BIGENDIAN
Copy link
Member

Choose a reason for hiding this comment

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

This fix is good!

#endif
}

uc->set_pc(uc, begin);
Copy link
Member

Choose a reason for hiding this comment

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

These two functions are not exactly the same unfortunately. You have to add:

                // force to quit execution and flush TB
                uc->quit_request = true;
                uc_emu_stop(uc);

@@ -164,9 +164,9 @@ static void test_arm_thumb_ite(void)
OK(uc_reg_write(uc, UC_ARM_REG_R3, &r_r3));

OK(uc_mem_map(uc, r_sp, 0x1000, UC_PROT_ALL));
r_r2 = 0x68;
r_r2 = cpu_to_le32(0x68);
Copy link
Member

Choose a reason for hiding this comment

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

A good way I think is to detect the host endianness directly and do this like:

#ifdef BIG_ENDIAN
r_r2 = bswap32(0x68)
#else
r_r2 = 0x68
#endif

@@ -172,8 +172,11 @@ static void test_x86_inc_dec_pxor(void)

TEST_CHECK(r_ecx == 0x1235);
TEST_CHECK(r_edx == 0x788f);
#ifndef HOST_WORDS_BIGENDIAN
Copy link
Member

Choose a reason for hiding this comment

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

Note again we don't use QEMU defines in the test files.

wtdcode added a commit to wtdcode/unicorn that referenced this pull request Oct 28, 2022
@wtdcode
Copy link
Member

wtdcode commented Oct 28, 2022

Fixed as of a40bf26

@wtdcode wtdcode closed this Oct 28, 2022
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