On 20/02/2008, Bert Freudenberg <bert at freudenbergs.de> wrote:
>
>  On Feb 20, 2008, at 16:48 , Igor Stasenko wrote:
>
>  > Hello,
>  > i'd like to mark code, located in Bitblt>>copyBits as buggy,
>  > regardless that it's currently working.
>  > A simple reason: if there is no such primitive, or primitive
>  > constantly fails, then code, located in method's body leading to
>  > infinite recursion.
>  > I consider this as a major bug, especially in such critical place,
>  > where correct behavior is at utmost importance.
>
>
> Firstly, copyBits is a non-optional primitive, so it should not fail
>  constantly.

You right, but it _is_ optional, on HydraVM :)
Consider my reasoning, why i think it's a bug:
- there is virtually impossible to strip image to prevent it from
using BitBlt/morphic.
Which means, that the only way left is to make primitives fail.
And now we got a problem: there is a code in image which thinks that
primitive can't constantly fail.
Maybe i'm biased but i don't like the code, which is too optimistic
about why primitive failed.

>
>  Secondly, which case in the body leads to direct recursion? At a
>  quick glance they seem fine.
>

I don't have a stack dump atm. Need to find it in my mail.

Another issue, which i found just today and like to be discussed is
Behavior>>basicNew: failure handling.

It is caused by a bug in ioScreenSize() vm function which returns a
size of enormous dimentions (59999 x 24555) . And VM simply refuses to
allocate bitmap of such size.

Here is the failure handling code:

	self isVariable ifFalse:
		[self error: self printString, ' cannot have variable sized instances'].
	(sizeRequested isInteger and: [sizeRequested >= 0]) ifTrue:
		["arg okay; space must be low."
		self environment signalLowSpace.
		^ self basicNew: sizeRequested  "retry if user proceeds"].
	self primitiveFailed

Since sizeRequested is integer and size requested is obviously > 0, it
enters the case
		self environment signalLowSpace.
		^ self basicNew: sizeRequested

I think it's a little brittle code.
IMHO 'self environment signalLowSpace' should block until user decides
what to do with given problem: abandon process or proceed.

In current implementation it looks like given code relies too much on
scheduler behavior: it expecting that by signaling low space semaphore
current process will be interrupted by higher priority process
immediately.

Or, if there was intent to inform user about low space, while
continuing running, in case, where primitive fails constantly, again
leading to infinite recursion.
In this case, i suppose, primitive should be invoked only once and if
it failed again, stop with error, and don't try torture VM/stack :)


-- 
Best regards,
Igor Stasenko AKA sig.