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

arm: treat ARM immediate values as unsigned #761

Closed
wants to merge 1 commit into from

Conversation

akihikodaki
Copy link
Contributor

It should be unsigned because:

  • It does arithmetic on addresses, which should be unsigned
  • Format strings have "%u" instead of "%d"

@@ -713,7 +713,7 @@ static void printOperand(MCInst *MI, unsigned OpNo, SStream *O)
} else if (MCOperand_isImm(Op)) {
unsigned int opc = MCInst_getOpcode(MI);

imm = (int32_t)MCOperand_getImm(Op);
imm = MCOperand_getImm(Op);
Copy link
Collaborator

Choose a reason for hiding this comment

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

you should typecast this to uint32_t. same for the code you modify below.

@aquynh
Copy link
Collaborator

aquynh commented Sep 2, 2016

where do you change the format string?

@akihikodaki
Copy link
Contributor Author

I have not changed any strings. They already have %u.

@akihikodaki
Copy link
Contributor Author

b8e66c9: Added casts according to #761 (comment).

@aquynh
Copy link
Collaborator

aquynh commented Sep 2, 2016 via email

@akihikodaki
Copy link
Contributor Author

You mean to add code to tests/test_arm.c?

@aquynh
Copy link
Collaborator

aquynh commented Sep 2, 2016 via email

It should be unsigned because:
* It does arithmetic on addresses, which should be unsigned
* Format strings have "%u" instead of "%d"
@akihikodaki
Copy link
Contributor Author

akihikodaki commented Sep 2, 2016

866b722: Added a testcase.

@@ -248,7 +248,7 @@ static void test()
},
};

uint64_t address = 0x1000;
uint64_t address = 0x80001000;
Copy link
Collaborator

Choose a reason for hiding this comment

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

on the current code (without your patch), 00 f0 10 e8 is blx #0x80001024. i dont see how this pull req changes the output?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It says blx #0x80001028 on my environment. By the way, libcapstone.so is copied to tests directory, but test_arm depends on libcapstone.so.3, which is installed in the system, and that confused me. Don't you have a similar issue?

$ cc -v
Using built-in specs.
COLLECT_GCC=cc
COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-pc-linux-gnu/6.1.1/lto-wrapper
Target: x86_64-pc-linux-gnu
Configured with: /build/gcc/src/gcc/configure --prefix=/usr --libdir=/usr/lib --libexecdir=/usr/lib --mandir=/usr/share/man --infodir=/usr/share/info --with-bugurl=https://bugs.archlinux.org/ --enable-languages=c,c++,ada,fortran,go,lto,objc,obj-c++ --enable-shared --enable-threads=posix --enable-libmpx --with-system-zlib --with-isl --enable-__cxa_atexit --disable-libunwind-exceptions --enable-clocale=gnu --disable-libstdcxx-pch --disable-libssp --enable-gnu-unique-object --enable-linker-build-id --enable-lto --enable-plugin --enable-install-libiberty --with-linker-hash-style=gnu --enable-gnu-indirect-function --disable-multilib --disable-werror --enable-checking=release
Thread model: posix
gcc version 6.1.1 20160802 (GCC)

Copy link
Collaborator

Choose a reason for hiding this comment

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

./make.sh should clean all the old code, then compiles again.

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