gnustep/libobjc2

maybe error in objc_msgSend.arm.S

Opened this issue · 4 comments

It's macro byte3, Thumb code is

ubfx   \dst, \src, #16, #8

ARM code is

and \dst, \src, #0xff00
lsr \dst, \dst, 16

Is it equivalent?
Why the #0xff00 isn't #0xff0000?

I think we can't materialise a constant that large in the and, so the sequence should actually be:

lsr \dst, \src, 16
and \dst, \dst, 0xff

Do you have an ARM system to test this on? I suspect that we've never hit that bug because programs with more than 2^16 distinct selectors are unlikely to fit in the RAM of such a machine (and are very rare - that path was broken on x86 for years without anyone noticing and, to-date, I'm only aware of one real-world program that hit this case). There is a test called ManyManySelectors that handles creating this many selectors, it should be quite simple to extend it to calling a method with the last selector value.

I think we can't materialise a constant that large in the and, so the sequence should actually be:

lsr \dst, \src, 16
and \dst, \dst, 0xff

Do you have an ARM system to test this on? I suspect that we've never hit that bug because programs with more than 2^16 distinct selectors are unlikely to fit in the RAM of such a machine (and are very rare - that path was broken on x86 for years without anyone noticing and, to-date, I'm only aware of one real-world program that hit this case). There is a test called ManyManySelectors that handles creating this many selectors, it should be quite simple to extend it to calling a method with the last selector value.

I agree with you that this is an extremely rare situation. What I can say now is to use an Android device, then annotate the irrelevant path, force the wrong logic, and see the results.

I'm happy to take a PR that fixes this, if you can test it.

Note that this code path is used only on ARMv6 without thumb support and earlier. The original Raspberry Pi and anything newer will use the bitfield extract instructions. We have these code paths for some dirt cheap Chinese Android phones that were still around five years ago, with ARMv5 CPUs.

Given that most of the devices that this code was intended to support are likely to be dead, it’s probably simpler to just drop support for suck old hardware. It’s unlikely any of them shipped with enough RAM to run a program that would trigger this bug.