|
From: Michael D. <mda...@qn...> - 2015-11-19 16:52:02
Attachments:
vg_add.w_shift.diff
|
Hello, When using GCC 5.2 I am seeing this assembly generated in some cases: add.w reg, sp, reg, lsl #4 The current limit is 3 though, so it was causing it to be caught as an unhandled instruction. Patch attached to bump the number from 3 to 4. Thanks, Mike --------------------------------------------------------------------- This transmission (including any attachments) may contain confidential information, privileged material (including material protected by the solicitor-client or other applicable privileges), or constitute non-public information. Any use of this information by anyone other than the intended recipient is prohibited. If you have received this transmission in error, please immediately reply to the sender and delete this information from your system. Use, dissemination, distribution, or reproduction of this transmission by unintended recipients is not authorized and may be unlawful. |
|
From: Florian K. <fl...@ei...> - 2015-11-24 20:01:28
|
I'm not familiar with arm. But I'm wondering whether allowing teh value 4 here is the right thing to do. Or, phrased differently: are there other values >4 possible, that we should handle as well? Florian On 19.11.2015 17:51, Michael Daniels wrote: > Hello, > > When using GCC 5.2 I am seeing this assembly generated in some cases: > > add.w reg, sp, reg, lsl #4 > > The current limit is 3 though, so it was causing it to be caught as an unhandled instruction. > > Patch attached to bump the number from 3 to 4. > > Thanks, > > Mike > --------------------------------------------------------------------- > This transmission (including any attachments) may contain confidential information, privileged material (including material protected by the solicitor-client or other applicable privileges), or constitute non-public information. Any use of this information by anyone other than the intended recipient is prohibited. If you have received this transmission in error, please immediately reply to the sender and delete this information from your system. Use, dissemination, distribution, or reproduction of this transmission by unintended recipients is not authorized and may be unlawful. > > > > ------------------------------------------------------------------------------ > > > > _______________________________________________ > Valgrind-developers mailing list > Val...@li... > https://lists.sourceforge.net/lists/listinfo/valgrind-developers > |
|
From: Michael D. <mda...@qn...> - 2015-11-24 20:14:46
|
> are there other values >4 possible, that we should handle as well? Shifts are 5 bits, so its possible this value could be as high as 31. Like the other patch, the check could/should probably just be removed altogether. It was just not clear why it was limited in the first place, so I only bumped it as needed. ________________________________________ From: Florian Krohm [fl...@ei...] Sent: Tuesday, November 24, 2015 2:48 PM To: Michael Daniels; val...@li... Subject: Re: [Valgrind-developers] [PATCH][VEX] Bump allowed shift value for "add.w reg, sp, reg, lsl #N" I'm not familiar with arm. But I'm wondering whether allowing teh value 4 here is the right thing to do. Or, phrased differently: are there other values >4 possible, that we should handle as well? Florian On 19.11.2015 17:51, Michael Daniels wrote: > Hello, > > When using GCC 5.2 I am seeing this assembly generated in some cases: > > add.w reg, sp, reg, lsl #4 > > The current limit is 3 though, so it was causing it to be caught as an unhandled instruction. > > Patch attached to bump the number from 3 to 4. > > Thanks, > > Mike > --------------------------------------------------------------------- > This transmission (including any attachments) may contain confidential information, privileged material (including material protected by the solicitor-client or other applicable privileges), or constitute non-public information. Any use of this information by anyone other than the intended recipient is prohibited. If you have received this transmission in error, please immediately reply to the sender and delete this information from your system. Use, dissemination, distribution, or reproduction of this transmission by unintended recipients is not authorized and may be unlawful. > > > > ------------------------------------------------------------------------------ > > > > _______________________________________________ > Valgrind-developers mailing list > Val...@li... > https://lists.sourceforge.net/lists/listinfo/valgrind-developers > |
|
From: Florian K. <fl...@ei...> - 2015-11-24 22:15:51
Attachments:
shift-limit.patch
|
OK, thanks for the explanation. Attached is the patch I'm proposing which removes the ad-hoc limitation and allows shift amounts up to and including 31. I looked at the downstream code and it has not problems handling such shift amounts. I'm copying Julian to he can chime in and offer an explanation as to why that limitation existed at all. If there is no yelling I'm going to commit this in the next couple of days. Florian On 24.11.2015 21:14, Michael Daniels wrote: >> are there other values >4 possible, that we should handle as well? > > Shifts are 5 bits, so its possible this value could be as high as 31. > Like the other patch, the check could/should probably just be > removed altogether. It was just not clear why it was limited in the > first place, so I only bumped it as needed. > ________________________________________ > From: Florian Krohm [fl...@ei...] > Sent: Tuesday, November 24, 2015 2:48 PM > To: Michael Daniels; val...@li... > Subject: Re: [Valgrind-developers] [PATCH][VEX] Bump allowed shift value for "add.w reg, sp, reg, lsl #N" > > I'm not familiar with arm. But I'm wondering whether allowing teh value > 4 here is the right thing to do. Or, phrased differently: are there > other values >4 possible, that we should handle as well? > > Florian > > > On 19.11.2015 17:51, Michael Daniels wrote: >> Hello, >> >> When using GCC 5.2 I am seeing this assembly generated in some cases: >> >> add.w reg, sp, reg, lsl #4 >> >> The current limit is 3 though, so it was causing it to be caught as an unhandled instruction. >> >> Patch attached to bump the number from 3 to 4. >> >> Thanks, >> >> Mike >> --------------------------------------------------------------------- >> This transmission (including any attachments) may contain confidential information, privileged material (including material protected by the solicitor-client or other applicable privileges), or constitute non-public information. Any use of this information by anyone other than the intended recipient is prohibited. If you have received this transmission in error, please immediately reply to the sender and delete this information from your system. Use, dissemination, distribution, or reproduction of this transmission by unintended recipients is not authorized and may be unlawful. >> >> >> >> ------------------------------------------------------------------------------ >> >> >> >> _______________________________________________ >> Valgrind-developers mailing list >> Val...@li... >> https://lists.sourceforge.net/lists/listinfo/valgrind-developers >> > > |
|
From: John R. <jr...@bi...> - 2015-11-24 23:17:51
|
On 11/24/2015 02:15 PM, Florian Krohm wrote: > OK, thanks for the explanation. Attached is the patch I'm proposing > which removes the ad-hoc limitation and allows shift amounts up to and > including 31. I looked at the downstream code and it has not problems > handling such shift amounts. > I'm copying Julian to he can chime in and offer an explanation as to why > that limitation existed at all. If there is no yelling I'm going to > commit this in the next couple of days. Shift counts up through 30 [== (2 * 15)] are legal and do work. There is a gray area if the effective absolute value (imm << shift) exceeds something like (1<<18) or (1<<20). Extending or truncating the stack by that much deserves a warning. The danger is "switching segments" without being aware of it. -- |
|
From: Florian K. <fl...@ei...> - 2015-11-29 15:34:59
|
Patch applied as VEX r3206 Florian On 24.11.2015 23:15, Florian Krohm wrote: > OK, thanks for the explanation. Attached is the patch I'm proposing > which removes the ad-hoc limitation and allows shift amounts up to and > including 31. I looked at the downstream code and it has not problems > handling such shift amounts. > I'm copying Julian to he can chime in and offer an explanation as to why > that limitation existed at all. If there is no yelling I'm going to > commit this in the next couple of days. > > Florian > > On 24.11.2015 21:14, Michael Daniels wrote: >>> are there other values >4 possible, that we should handle as well? >> >> Shifts are 5 bits, so its possible this value could be as high as 31. >> Like the other patch, the check could/should probably just be >> removed altogether. It was just not clear why it was limited in the >> first place, so I only bumped it as needed. >> ________________________________________ >> From: Florian Krohm [fl...@ei...] >> Sent: Tuesday, November 24, 2015 2:48 PM >> To: Michael Daniels; val...@li... >> Subject: Re: [Valgrind-developers] [PATCH][VEX] Bump allowed shift value for "add.w reg, sp, reg, lsl #N" >> >> I'm not familiar with arm. But I'm wondering whether allowing teh value >> 4 here is the right thing to do. Or, phrased differently: are there >> other values >4 possible, that we should handle as well? >> >> Florian >> >> >> On 19.11.2015 17:51, Michael Daniels wrote: >>> Hello, >>> >>> When using GCC 5.2 I am seeing this assembly generated in some cases: >>> >>> add.w reg, sp, reg, lsl #4 >>> >>> The current limit is 3 though, so it was causing it to be caught as an unhandled instruction. >>> >>> Patch attached to bump the number from 3 to 4. >>> >>> Thanks, >>> >>> Mike |
|
From: Julian S. <js...@ac...> - 2015-12-28 16:44:17
|
[Arriving very late at the party] What this does is Reg1 = SP + (Reg2 << shift-amount) If you consider what that could possibly be used for .. it seems like it would compute the end address of a stack allocated array of Reg2 items, each of size (1 << shift-amount). I suspect that's why we've only ever seen shift amounts of 0,1,2,3 up till this point. Given that these array items are not likely to be very big in practice, and given that I have great difficulty in understanding the ARM ARM limitations on shift amounts for this particular instruction, I'd be more comfortable increasing the allowable shift amounts up to say 8 (allowing element sizes of up to 256) but not more than that. If it breaks again we could increase it, but given that it's taken years even for the case '4' to appear, I think that's fairly unlikely. In summary, I am inclined to change the <= 31 to <= 8. J On 24/11/15 23:15, Florian Krohm wrote: > OK, thanks for the explanation. Attached is the patch I'm proposing > which removes the ad-hoc limitation and allows shift amounts up to and > including 31. I looked at the downstream code and it has not problems > handling such shift amounts. > I'm copying Julian to he can chime in and offer an explanation as to why > that limitation existed at all. If there is no yelling I'm going to > commit this in the next couple of days. > > Florian > > On 24.11.2015 21:14, Michael Daniels wrote: >>> are there other values >4 possible, that we should handle as well? >> >> Shifts are 5 bits, so its possible this value could be as high as 31. >> Like the other patch, the check could/should probably just be >> removed altogether. It was just not clear why it was limited in the >> first place, so I only bumped it as needed. >> ________________________________________ >> From: Florian Krohm [fl...@ei...] >> Sent: Tuesday, November 24, 2015 2:48 PM >> To: Michael Daniels; val...@li... >> Subject: Re: [Valgrind-developers] [PATCH][VEX] Bump allowed shift value for "add.w reg, sp, reg, lsl #N" >> >> I'm not familiar with arm. But I'm wondering whether allowing teh value >> 4 here is the right thing to do. Or, phrased differently: are there >> other values >4 possible, that we should handle as well? >> >> Florian >> >> >> On 19.11.2015 17:51, Michael Daniels wrote: >>> Hello, >>> >>> When using GCC 5.2 I am seeing this assembly generated in some cases: >>> >>> add.w reg, sp, reg, lsl #4 >>> >>> The current limit is 3 though, so it was causing it to be caught as an unhandled instruction. >>> >>> Patch attached to bump the number from 3 to 4. >>> >>> Thanks, >>> >>> Mike >>> --------------------------------------------------------------------- >>> This transmission (including any attachments) may contain confidential information, privileged material (including material protected by the solicitor-client or other applicable privileges), or constitute non-public information. Any use of this information by anyone other than the intended recipient is prohibited. If you have received this transmission in error, please immediately reply to the sender and delete this information from your system. Use, dissemination, distribution, or reproduction of this transmission by unintended recipients is not authorized and may be unlawful. >>> >>> >>> >>> ------------------------------------------------------------------------------ >>> >>> >>> >>> _______________________________________________ >>> Valgrind-developers mailing list >>> Val...@li... >>> https://lists.sourceforge.net/lists/listinfo/valgrind-developers >>> >> >> > |
|
From: Florian K. <fl...@ei...> - 2015-12-30 10:45:24
|
On 28.12.2015 17:25, Julian Seward wrote: > > [Arriving very late at the party] > > What this does is Reg1 = SP + (Reg2 << shift-amount) > > If you consider what that could possibly be used for .. I'm not considering it, because that is not our job. Our job is to decide whether the insn is valid and if so, emulate it. I realise that it may be difficult to figure out all the constraints regarding shift amounts. But that does not give us the right to terminate the process. Most of the insns we see were generated by compilers; so the probability of such insns to be valid is very high and we should not second guess it. What we could do is to perhaps issue an emulation warning and continue. Florian > it seems like > it would compute the end address of a stack allocated array of Reg2 > items, each of size (1 << shift-amount). I suspect that's why we've > only ever seen shift amounts of 0,1,2,3 up till this point. > > Given that these array items are not likely to be very big in practice, > and given that I have great difficulty in understanding the ARM ARM > limitations on shift amounts for this particular instruction, I'd be > more comfortable increasing the allowable shift amounts up to say 8 > (allowing element sizes of up to 256) but not more than that. If it > breaks again we could increase it, but given that it's taken years > even for the case '4' to appear, I think that's fairly unlikely. > > In summary, I am inclined to change the <= 31 to <= 8. > > J > > On 24/11/15 23:15, Florian Krohm wrote: >> OK, thanks for the explanation. Attached is the patch I'm proposing >> which removes the ad-hoc limitation and allows shift amounts up to and >> including 31. I looked at the downstream code and it has not problems >> handling such shift amounts. >> I'm copying Julian to he can chime in and offer an explanation as to why >> that limitation existed at all. If there is no yelling I'm going to >> commit this in the next couple of days. >> >> Florian >> >> On 24.11.2015 21:14, Michael Daniels wrote: >>>> are there other values >4 possible, that we should handle as well? >>> >>> Shifts are 5 bits, so its possible this value could be as high as 31. >>> Like the other patch, the check could/should probably just be >>> removed altogether. It was just not clear why it was limited in the >>> first place, so I only bumped it as needed. >>> ________________________________________ >>> From: Florian Krohm [fl...@ei...] >>> Sent: Tuesday, November 24, 2015 2:48 PM >>> To: Michael Daniels; val...@li... >>> Subject: Re: [Valgrind-developers] [PATCH][VEX] Bump allowed shift value for "add.w reg, sp, reg, lsl #N" >>> >>> I'm not familiar with arm. But I'm wondering whether allowing teh value >>> 4 here is the right thing to do. Or, phrased differently: are there >>> other values >4 possible, that we should handle as well? >>> >>> Florian >>> >>> >>> On 19.11.2015 17:51, Michael Daniels wrote: >>>> Hello, >>>> >>>> When using GCC 5.2 I am seeing this assembly generated in some cases: >>>> >>>> add.w reg, sp, reg, lsl #4 >>>> >>>> The current limit is 3 though, so it was causing it to be caught as an unhandled instruction. >>>> >>>> Patch attached to bump the number from 3 to 4. >>>> >>>> Thanks, >>>> >>>> Mike >>>> --------------------------------------------------------------------- >>>> This transmission (including any attachments) may contain confidential information, privileged material (including material protected by the solicitor-client or other applicable privileges), or constitute non-public information. Any use of this information by anyone other than the intended recipient is prohibited. If you have received this transmission in error, please immediately reply to the sender and delete this information from your system. Use, dissemination, distribution, or reproduction of this transmission by unintended recipients is not authorized and may be unlawful. >>>> >>>> >>>> >>>> ------------------------------------------------------------------------------ >>>> >>>> >>>> >>>> _______________________________________________ >>>> Valgrind-developers mailing list >>>> Val...@li... >>>> https://lists.sourceforge.net/lists/listinfo/valgrind-developers >>>> >>> >>> >> > > |
|
From: Julian S. <js...@ac...> - 2016-10-19 16:45:05
|
This was fixed in VEX r3206, generalised for any shift amount from 0 to 31 inclusive. J > When using GCC 5.2 I am seeing this assembly generated in some cases: > > add.w reg, sp, reg, lsl #4 > > The current limit is 3 though, so it was causing it to be caught as an unhandled instruction. > > Patch attached to bump the number from 3 to 4. |