Smalltalk › Squeak › Squeak VM

dangerous translation of MiscPrimitivePlugin code

_‹ Previous Topic Next Topic _›

Classic

List

Threaded

6 messages Options

Nicolas Cellier

dangerous translation of MiscPrimitivePlugin code

Hi,

I don't know if it was reported yet, but we've got some nasty UB detected by C Compiler warning in primitiveFindSubstring:

../../src/plugins/MiscPrimitivePlugin/MiscPrimitivePlugin.c:745:53: warning: unsequenced modification and access to 'index' [-Wunsequenced]
while ((body[startIndex + index]) == (key[(index += 1)])) {

Effectively, the sequence is well defined in Smalltalk, not in C.
See String>>#findSubstring:in:startingAt:matchTable:

(body at: startIndex + index) == (key at: (index := index + 1))

Since the CCodeGenerator does not handle this,

we should better decompose the Smalltalk code into several instructions.

Hint: mind the C compiler warnings, some of them are true positive!

Half my changes/fixes are driven by them.

Nicolas

Levente Uzonyi

Re: dangerous translation of MiscPrimitivePlugin code

Hi Nicolas,

That version of the method was present for 3 weeks in the Trunk. So,
whoever generated the sources hasn't updated their image since the middle
of April.

Levente

Nicolas Cellier

Re: dangerous translation of MiscPrimitivePlugin code

2016-07-08 23:07 GMT+02:00 Levente Uzonyi <[hidden email]>:

Hi Nicolas,

That version of the method was present for 3 weeks in the Trunk. So, whoever generated the sources hasn't updated their image since the middle of April.

Levente

thanks Levente.

That must be because the MiscPrimitivePlugin code is not part of VMMaker package but rather spreaded in trunk packages.

I don't think that the scripts for building VMMaker images are going thru squeak trunk update...

Nicolas

timrowledge

Re: dangerous translation of MiscPrimitivePlugin code

In reply to this post by Nicolas Cellier

> On 08-07-2016, at 1:49 PM, Nicolas Cellier <[hidden email]> wrote:
>
> Hi,
> I don't know if it was reported yet, but we've got some nasty UB detected by C Compiler warning in primitiveFindSubstring:

A long, long, time ago when Andreas & I were first making the plugins stuff we were determined that miscprimitiveplugin should be a very short-lived thing. Anyone feeling the urge to cleanup/out that ugliness should please do so asap.

tim
--
tim Rowledge; [hidden email]; http://www.rowledge.org/tim
From C:\*.* to shining C:\*.*

Eliot Miranda-2

Re: dangerous translation of MiscPrimitivePlugin code

On Fri, Jul 8, 2016 at 3:00 PM, tim Rowledge <[hidden email]> wrote:

> On 08-07-2016, at 1:49 PM, Nicolas Cellier <[hidden email]> wrote:
>
> Hi,
> I don't know if it was reported yet, but we've got some nasty UB detected by C Compiler warning in primitiveFindSubstring:

A long, long, time ago when Andreas & I were first making the plugins stuff we were determined that miscprimitiveplugin should be a very short-lived thing. Anyone feeling the urge to cleanup/out that ugliness should please do so asap.

The first thing to do is a performance measurement to find out which primitives still pay their way. Any that don't offer a substantial speedup should be discarded. Substantial would be say -33%. Sista is making excellent progress and will be here soon enough that code like MiscprimitivePlugin's primitives will be optimised well.

tim
--
tim Rowledge; [hidden email]; http://www.rowledge.org/tim
>From C:\*.* to shining C:\*.*

_,,,^..^,,,_

best, Eliot

David T. Lewis

Re: dangerous translation of MiscPrimitivePlugin code

In reply to this post by timrowledge

On Fri, Jul 08, 2016 at 03:00:58PM -0700, tim Rowledge wrote:
>
>
> > On 08-07-2016, at 1:49 PM, Nicolas Cellier <[hidden email]> wrote:
> >
> > Hi,
> > I don't know if it was reported yet, but we've got some nasty UB detected by C Compiler warning in primitiveFindSubstring:
>
> A long, long, time ago when Andreas & I were first making the plugins stuff we were determined that miscprimitiveplugin should be a very short-lived thing. Anyone feeling the urge to cleanup/out that ugliness should please do so asap.
>

It's probably time for doing this. The included methods are a nice way to
show the concept of Smalltalk methods being translated to C for efficiency,
but they are rather inconvenient with regard to version control.

In addition to MiscPrimitivePlugin, we also have SoundGeneratorPlugin and
ADPCMCodecPlugin that use included methods. IncludedMethodsTest in VMMaker
is my attempt to keep track of these.

I presume that the sensible thing to do would be to just copy the necessary
methods directly into the plugins under different names and generate them
directly as with any other primitive. As Sista makes the primitives redundant,
just stop calling them.

Dave