<div dir="ltr">Hi Levente,<div><br></div><div>[I'm sorry, I apparently forgot to hit send some weeks ago]<br><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Feb 25, 2018 at 4:12 PM, Levente Uzonyi <span dir="ltr"><<a href="mailto:leves@caesar.elte.hu" target="_blank">leves@caesar.elte.hu</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
Hi Eliot,<br>
<br>
I went through the primitives and found the following issues:<br></blockquote><div><br></div><div>Thanks for these! I've fixed most of them. Justification for the unchanged ones below. Feel free to twist my arm if any still look wrong :-)</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
primitiveCompareString<br>
- size of the order variable should be checked to be 256 as it is done in primitiveFindFirstInString<br>
primitiveCompressToByteArray<br>
- ba should be bounds checked<br>
- bm is int*, but it probably should be unsigned int* because of the right shifts<br>
primitiveDecompressFromByteArr<wbr>ay<br>
- bm is int*, but it should probably be unsigned int* as the method stores unsigned ints (data) into it<br>
- index should be bounds checked (1 .. end)<br></blockquote><div><br></div><div>It gets checked anyway with the comparison against pastEnd</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
- i, k, end and pastEnd should be unsigned int<br></blockquote><div><br></div><div>That's OK. In neither V3 nor Spur can objects have more than 1Gb fields so in practice indices can never go negative.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
primitiveFindFirstInString<br>
- inclustionMap should be unsigned char* (in practice it's a ByteArray with 0-1 values)<br></blockquote><div><br></div><div>Since its bytes only get tested for 0 I left this unchanged.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
primitiveIndexOfAsciiInString<br>
- start can be 0 or negative. Either the loop should start from (start - 1 max: 0) or there should be an error if start is 0 or negative<br>
- anInteger should be bounds checked (0 .. 255)<br></blockquote><div><br></div><div>Not sure about this. It is a waste of time to check, but if one givers it an out-of-bound value the result will still be valid, and if the primitive failed it would be no better. So I left it alone.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
primitiveStringHash<br>
- speciesHash should probably be unsigned int<br></blockquote><div><br></div><div>Again cuz of the masking its ok to leave this.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
primitiveTranslateStringWithTa<wbr>ble<br>
- start and stop need bounds checks (0 .. aString size - 1)<br>
- the size of table should be checked to be 256<span class="m_-5564311271928322486HOEnZb"><font color="#888888"><br>
<br>
Levente</font></span><div class="m_-5564311271928322486HOEnZb"><div class="m_-5564311271928322486h5"><br>
<br>
On Thu, 22 Feb 2018, Eliot Miranda wrote:<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Hi Levente,<br>
<br>
I really appreciate you taking a look. I will say that the rewrite tried the stay as close to the originals while changing 1-relative indexing to 0-relative. But I think we can also improve on the originals. Indeed I spotted and fixed a potential bounds violation in primitiveConvert8Bit that only checked the size of one of the two array arguments.<br>
<br>
So can I ask you to read the primitives in the previous commit too? Let's polish these a little :-)<br>
<br>
_,,,^..^,,,_ (phone)<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On Feb 22, 2018, at 3:14 PM, Levente Uzonyi <<a href="mailto:leves@caesar.elte.hu" target="_blank">leves@caesar.elte.hu</a>> wrote:<br>
<br>
Just had a look at this snippet, and wrote some quick inline comments.<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On Thu, 22 Feb 2018, <a href="mailto:commits@source.squeak.org" target="_blank">commits@source.squeak.org</a> wrote:<br>
<br>
Eliot Miranda uploaded a new version of VMMaker to project VM Maker:<br>
<a href="http://source.squeak.org/VMMaker/VMMaker.oscog-eem.2338.mcz" rel="noreferrer" target="_blank">http://source.squeak.org/VMMak<wbr>er/VMMaker.oscog-eem.2338.mcz</a><br>
<br>
==================== Summary ====================<br>
<br>
Name: VMMaker.oscog-eem.2338<br>
Author: eem<br>
Time: 22 February 2018, 10:25:17.794166 am<br>
UUID: 1672c678-41b4-41f2-a061-e20156<wbr>2fc75c<br>
Ancestors: VMMaker.oscog-eem.2337<br>
<br>
Fix a slip in primitiveDecompressFromByteArr<wbr>ay caught by IncludedMethodsTests. Thank you David!<br>
<br>
=============== Diff against VMMaker.oscog-eem.2337 ===============<br>
<br>
Item was changed:<br>
----- Method: MiscPrimitivePlugin>>primitive<wbr>DecompressFromByteArray (in category 'primitives') -----<br>
primitiveDecompressFromByteArr<wbr>ay<br>
"Bitmap decompress: bm fromByteArray: ba at: index"<br>
<export: true><br>
| bm ba index i anInt code data end k n pastEnd |<br>
<var: 'ba' type: #'unsigned char *'><br>
<var: 'bm' type: #'int *'><br>
<var: 'anInt' type: #'unsigned int'><br>
<var: 'code' type: #'unsigned int'><br>
<var: 'data' type: #'unsigned int'><br>
<var: 'n' type: #'unsigned int'><br>
</blockquote>
<br>
Shouldn't i be declared as unsigned int too? It's used for indexing.<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
bm := self cCode: [interpreterProxy arrayValueOf: (interpreterProxy stackValue: 2)]<br>
inSmalltalk: [interpreterProxy<br>
cCoerce: (interpreterProxy arrayValueOf: (interpreterProxy stackValue: 2))<br>
to: #'int *'].<br>
(interpreterProxy isOopImmutable: (interpreterProxy stackValue: 2)) ifTrue:<br>
[^interpreterProxy primitiveFailFor: PrimErrNoModification].<br>
(interpreterProxy isBytes: (interpreterProxy stackValue: 1)) ifFalse: [^interpreterProxy primitiveFail].<br>
ba := interpreterProxy firstIndexableField: (interpreterProxy stackValue: 1).<br>
index := interpreterProxy stackIntegerValue: 0.<br>
</blockquote>
<br>
Can index be negative or 0? If yes, then the value of i will be negative (unless it's implicit type is unsigned) and that will result in out of bounds indexing into ba a few lines below here.<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
interpreterProxy failed ifTrue: [^nil].<br>
i := index - 1.<br>
k := 0.<br>
end := interpreterProxy sizeOfSTArrayFromCPrimitive: ba.<br>
pastEnd := interpreterProxy sizeOfSTArrayFromCPrimitive: bm.<br>
[i < end] whileTrue:<br>
[anInt := ba at: i.<br>
i := i + 1.<br>
anInt <= 223 ifFalse:<br>
[anInt <= 254<br>
ifTrue:<br>
[anInt := anInt - 224 * 256 + (ba at: i).<br>
</blockquote>
<br>
I would use bitShift: 8 instead of * 256, just to be consistent with the rest of the method.<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
i := i + 1]<br>
ifFalse:<br>
[anInt := 0.<br>
1 to: 4 by: 1 do:<br>
[ :j | anInt := (anInt bitShift: 8) + (ba at: i).<br>
i := i + 1]]].<br>
n := anInt >> 2.<br>
+ k + n > pastEnd ifTrue: [^interpreterProxy primitiveFail].<br>
- k + n >= pastEnd ifTrue: [^interpreterProxy primitiveFail].<br>
code := anInt bitAnd: 3.<br>
"code = 0 ifTrue: [nil]."<br>
code = 1 ifTrue:<br>
[data := ba at: i.<br>
i := i + 1.<br>
data := data bitOr: (data bitShift: 8).<br>
data := data bitOr: (data bitShift: 16).<br>
</blockquote>
<br>
I don't know what the point of this part of the code is, but I think the above 4 lines could read:<br>
<br>
[data := 16r10101 * (ba at: i).<br>
i := i + 1.<br>
<br>
Levente<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
1 to: n do:<br>
[ :j |<br>
bm at: k put: data.<br>
k := k + 1]].<br>
code = 2 ifTrue:<br>
[data := 0.<br>
1 to: 4 do:<br>
[ :j |<br>
data := (data bitShift: 8) bitOr: (ba at: i).<br>
i := i + 1].<br>
1 to: n do:<br>
[ :j |<br>
bm at: k put: data.<br>
k := k + 1]].<br>
code = 3 ifTrue:<br>
[1 to: n do:<br>
[ :m |<br>
data := 0.<br>
1 to: 4 do:<br>
[ :j |<br>
data := (data bitShift: 8) bitOr: (ba at: i).<br>
i := i + 1].<br>
bm at: k put: data.<br>
k := k + 1]]].<br>
interpreterProxy pop: interpreterProxy methodArgumentCount!<br>
</blockquote></blockquote></blockquote>
</div></div></blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="m_-5564311271928322486gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div><span style="font-size:small;border-collapse:separate"><div>_,,,^..^,,,_<br></div><div>best, Eliot</div></span></div></div></div>
</div></div></div>