Hi folks,
Is not posible to find all the buggy casts (between unsigned and signed ints) using this compiler flags?
Anyone did try it?
Cheers,
-- Diego
Diego Gomez Deck wrote:
Is not posible to find all the buggy casts (between unsigned and signed ints) using this compiler flags?
I'm actually not sure.
Anyone did try it?
Once in a blue moon, yes. However, the number of warnings was so overwhelming that it felt to laborious a task (in particular considering that often you need to find your way backwards from the C code to the slang code and that inlining makes the problem worse).
Cheers, - Andreas
On Fri, Mar 16, 2007 at 10:23:00AM -0700, Andreas Raab wrote:
Diego Gomez Deck wrote:
Is not posible to find all the buggy casts (between unsigned and signed ints) using this compiler flags?
I'm actually not sure.
Anyone did try it?
Once in a blue moon, yes. However, the number of warnings was so overwhelming that it felt to laborious a task (in particular considering that often you need to find your way backwards from the C code to the slang code and that inlining makes the problem worse).
Actually, I would not mind putting some time into this if there was a way to organize the work such that any changes I identified could be properly vetted. I recently came up with a way to browse generated C and inlined C code in Squeak (*), which makes the problem seem considerably less intimidating. I've also done the 64 bit updates for some plugins (OSPP et al), so I have a passing familiarity with the issues.
The suggestion of using -pedantic is a good one, but I'll note that there are compiler warnings generated simply by compiling the VM on a 64 bit host. Making those warnings go away would be a good first objective.
Dave
David T. Lewis wrote:
Actually, I would not mind putting some time into this if there was a way to organize the work such that any changes I identified could be properly vetted.
If you can keep the changes in reasonably small chunks, I think this would be doable. Just try to avoid the one-size-fixes-all approach of throwing several hundred changed methods at a single point in time.
I recently came up with a way to browse generated C and inlined C code in Squeak (*), which makes the problem seem considerably less intimidating. I've also done the 64 bit updates for some plugins (OSPP et al), so I have a passing familiarity with the issues.
I doubt that looking at the generated code will help much in general unless you can find a way to run gcc over these methods and have it show us directly the things it doesn't like. As a matter of fact, I think a non-inlined VM would be a much better starting point since it will only report each problem once and not every time it trips over an inlined variant.
Cheers, - Andreas
On Sat, Mar 17, 2007 at 02:54:52PM -0700, Andreas Raab wrote:
David T. Lewis wrote:
Actually, I would not mind putting some time into this if there was a way to organize the work such that any changes I identified could be properly vetted.
If you can keep the changes in reasonably small chunks, I think this would be doable. Just try to avoid the one-size-fixes-all approach of throwing several hundred changed methods at a single point in time.
Agreed.
I recently came up with a way to browse generated C and inlined C code in Squeak (*), which makes the problem seem considerably less intimidating. I've also done the 64 bit updates for some plugins (OSPP et al), so I have a passing familiarity with the issues.
I doubt that looking at the generated code will help much in general unless you can find a way to run gcc over these methods and have it show us directly the things it doesn't like. As a matter of fact, I think a non-inlined VM would be a much better starting point since it will only report each problem once and not every time it trips over an inlined variant.
Agreed. Although I do like being able to inspect the generated C code with or without inlining immediately after making a change. Perhaps it's just because the computer I normally use is rather slow, and I like having some immediate feedback.
Dave
On Sat, Mar 17, 2007 at 02:54:52PM -0700, Andreas Raab wrote:
David T. Lewis wrote:
Actually, I would not mind putting some time into this if there was a way to organize the work such that any changes I identified could be properly vetted.
If you can keep the changes in reasonably small chunks, I think this would be doable. Just try to avoid the one-size-fixes-all approach of throwing several hundred changed methods at a single point in time.
I have worked though ObjectMemory and Interpreter to identify and fix the methods that do oop comparisons (#>, #>=, #<, #<=) with signed integer (sqInt) operands. These takes the form of either explicit type declarations of variables to usqInt, or type casts for the comparison operands. The type casts are all done within four new methods to minimize clutter and document the intent of the casts. These methods are inlined by the slang generator, and none of the changes prevent inlining of methods that currently are inlined. There is also a related change to pointerForOop() in sqMemoryAccess.h, which was doing an unintended sign extension when converting 4 byte sqInt oop values to 8 byte machine addresses on 64 bit host/32 bit image.
I do not have a machine that exhibits the problem of object memory addresses with negative integer values, reported on some newer Linux machines. For that reason, I cannot confirm that my changes actually resolve that problem. However, I could not spot any issues in the VM other than the typing and oop comparison ones, so I think there is a reasonable chance that these updates will make that problem go away.
Any preferences as to how and where to make these updates available? I can put them into about a half dozen change sets, or provide MCZ files, or stick them on Mantis, or all of the above. Possibly the easiest thing is to just send the change sets to this list?
Dave
Wow. Nice work. I can't speak for the others but since we have the problem very practically at Qwaq I'd be willing to test drive these changes for a while and see if they fix the problems. The best way to do this would probably be to post change sets to this list (this allows me to go over them method by method more easily).
Thanks for all the work!
Cheers, - Andreas
David T. Lewis wrote:
On Sat, Mar 17, 2007 at 02:54:52PM -0700, Andreas Raab wrote:
David T. Lewis wrote:
Actually, I would not mind putting some time into this if there was a way to organize the work such that any changes I identified could be properly vetted.
If you can keep the changes in reasonably small chunks, I think this would be doable. Just try to avoid the one-size-fixes-all approach of throwing several hundred changed methods at a single point in time.
I have worked though ObjectMemory and Interpreter to identify and fix the methods that do oop comparisons (#>, #>=, #<, #<=) with signed integer (sqInt) operands. These takes the form of either explicit type declarations of variables to usqInt, or type casts for the comparison operands. The type casts are all done within four new methods to minimize clutter and document the intent of the casts. These methods are inlined by the slang generator, and none of the changes prevent inlining of methods that currently are inlined. There is also a related change to pointerForOop() in sqMemoryAccess.h, which was doing an unintended sign extension when converting 4 byte sqInt oop values to 8 byte machine addresses on 64 bit host/32 bit image.
I do not have a machine that exhibits the problem of object memory addresses with negative integer values, reported on some newer Linux machines. For that reason, I cannot confirm that my changes actually resolve that problem. However, I could not spot any issues in the VM other than the typing and oop comparison ones, so I think there is a reasonable chance that these updates will make that problem go away.
Any preferences as to how and where to make these updates available? I can put them into about a half dozen change sets, or provide MCZ files, or stick them on Mantis, or all of the above. Possibly the easiest thing is to just send the change sets to this list?
Dave
The attached zip contains six change sets and an update for sqMemoryAccess.h. The changes are intended to resolve problems with oop variable declarations and comparison operations that may occur on platforms that assign object memory to high virtual memory address values.
Dave
The change set preambles are:
=== VmUpdates-1001-dtl.1.cs === Change Set: VmUpdates-1001-dtl Date: 23 April 2007 Author: David T. Lewis
Remove several unreferenced primitives. These have no apparent utility in the current VM. They contain type casts that are probably incorrect, that that produce compiler warnings on 64 bit host/32 bit image VM builds.
This is the first of several change sets that address object memory addressing issues, particularly for host machines that may assign object memory addresses in the high order range of an unsigned 32 bit address space. The changes address oop comparison and type conversion within the object memory and interpreter.
Prerequisites are: 1) VMMaker 3.8b6 (from file VMMaker-tpr.58.mcz). 2) Update to sqMemoryAccess.h with the following changes: 85c85 < static inline char *pointerForOop(sqInt oop) { return sqMemoryBase + oop; } ---
static inline char *pointerForOop(usqInt oop) { return sqMemoryBase + oop; }
109,110c109,110 < # define pointerForOop(oop) ((char *)(sqMemoryBase + (oop))) < # define oopForPointer(ptr) ((sqInt)(ptr)) ---
# define pointerForOop(oop) ((char *)(sqMemoryBase + ((usqInt)(oop)))) # define oopForPointer(ptr) ((sqInt)(((char *)(ptr)) - (sqMemoryBase)))
Other patches are required for various 64 bit host/image combinations, but are not directly relevent to this series of updates. See Mantis for details.
=== VmUpdates-1002-dtl.1.cs === Change Set: VmUpdates-1002-dtl Date: 23 April 2007 Author: David T. Lewis
Declare globals for oops in ObjectMemory and Interpreter as type usqInt.
=== VmUpdates-1003-dtl.1.cs === Change Set: VmUpdates-1003-dtl Date: 23 April 2007 Author: David T. Lewis
Add oop comparison methods. ObjectMemory>>oop:isGreaterThan: ObjectMemory>>oop:isGreaterThanOrEqual: ObjectMemory>>oop:isLessThan: ObjectMemory>>oop:isLessThanOrEqual:
These use #cCoerce:to: to cast their arguments to unsigned usqInt. They are inlined during C translation so performance is not impacted.
Notes: Any explicit C variable declarations in methods will prevent the methods from being inlined. These four new methods, when implemented in ObjectMemory, do the required type casts without disabling inlining. Also, the methods must be implemented here rather than in Object in order for the inlining to work.
=== VmUpdates-1004-dtl.1.cs === Change Set: VmUpdates-1004-dtl Date: 23 April 2007 Author: David T. Lewis
Use new oop comparison methods throughout ObjectMemory wherever necessary to ensure unsigned operands. In some methods, the original comparison operators are used if referencing globals declared as usqInt, or if the methods are not inlined so that local declarations may be used.
Updated #sufficientSpaceAfterGC:, #sufficientSpaceToAllocate: and #allocateChunk to use the new methods rather than Ian's original casts.
=== VmUpdates-1005-dtl.1.cs === Change Set: VmUpdates-1005-dtl Date: 23 April 2007 Author: David T. Lewis
Use new oop comparison methods throughout Interpreter wherever necessary to ensure unsigned operands. In some methods, the original comparison operators are used if referencing globals declared as usqInt, or if the methods are not inlined so that local declarations may be used.
Update #stObject:at: and #stObject:at:put: to use the new comparison methods rather than Ian's original casts.
=== VmUpdates-1006-dtl.1.cs === Change Set: VmUpdates-1006-dtl Date: 23 April 2007 Author: David T. Lewis
Removed unnecessary type cast in #biasToGrowCheckGCLimit. JMM please double-check this and make sure I did not mess up your original intent.
Ok, I was working with Craig a bit on this and I noticed for example.
usqInt endOfMemory;
sqInt fwdBlock2;
/* begin restoreHeadersAfterForwardBecome: */ fwdBlock2 = ((foo->endOfMemory + BaseHeaderSize) + 7) & (WordMask - 7); flag("Dan"); fwdBlock2 += BytesPerWord * 4;
Really shouldn't one consider that if a unsigned value gets assigned to a signed integer one should really consider that a possible problem?
Needless to say I believe the code above won't work as intended.
On Apr 25, 2007, at 3:59 AM, David T. Lewis wrote:
The attached zip contains six change sets and an update for sqMemoryAccess.h.
-- ======================================================================== === John M. McIntosh johnmci@smalltalkconsulting.com Corporate Smalltalk Consulting Ltd. http://www.smalltalkconsulting.com ======================================================================== ===
On May 7, 2007, at 17:25 , John M McIntosh wrote:
Ok, I was working with Craig a bit on this and I noticed for example.
usqInt endOfMemory;
sqInt fwdBlock2;
/* begin restoreHeadersAfterForwardBecome: */ fwdBlock2 = ((foo->endOfMemory + BaseHeaderSize) + 7) & (WordMask
- 7); flag("Dan"); fwdBlock2 += BytesPerWord * 4;
Really shouldn't one consider that if a unsigned value gets assigned to a signed integer one should really consider that a possible problem?
Needless to say I believe the code above won't work as intended.
Shooting from the hip and all, but I'd say thanks to two's complement this actually should work. Far from pretty or desirable, but it should generate the same code (unless the C compiler inserts specific checks which it should not and AFAIK it does not).
- Bert -
Ah well something else wrong I guess.
let's see -0x0A is 0xFFFFFFFF6
mmm add say + 4 is 0xFFFFFFFA or (-0x06)
need more coffee.
On May 7, 2007, at 6:35 PM, Bert Freudenberg wrote:
On May 7, 2007, at 17:25 , John M McIntosh wrote:
Ok, I was working with Craig a bit on this and I noticed for example.
usqInt endOfMemory;
sqInt fwdBlock2;
/* begin restoreHeadersAfterForwardBecome: */ fwdBlock2 = ((foo->endOfMemory + BaseHeaderSize) + 7) &(WordMask - 7); flag("Dan"); fwdBlock2 += BytesPerWord * 4;
Really shouldn't one consider that if a unsigned value gets assigned to a signed integer one should really consider that a possible problem?
Needless to say I believe the code above won't work as intended.
Shooting from the hip and all, but I'd say thanks to two's complement this actually should work. Far from pretty or desirable, but it should generate the same code (unless the C compiler inserts specific checks which it should not and AFAIK it does not).
- Bert -
-- ======================================================================== === John M. McIntosh johnmci@smalltalkconsulting.com Corporate Smalltalk Consulting Ltd. http://www.smalltalkconsulting.com ======================================================================== ===
On May 7, 2007, at 6:35 PM, Bert Freudenberg wrote:
On May 7, 2007, at 17:25 , John M McIntosh wrote:
Ok, I was working with Craig a bit on this and I noticed for example.
Ok, well I think...
markAndTraceDiscardingStaleMethods
if (oop >= foo->youngStart) { header = header | MarkBit; }
sqInt markAllActiveMethods(void) {
while (oop < foo->endOfMemory) {
less coffee someone can decide if wrong/right...
-- ======================================================================== === John M. McIntosh johnmci@smalltalkconsulting.com Corporate Smalltalk Consulting Ltd. http://www.smalltalkconsulting.com ======================================================================== ===
Those are part of Spoon. The first of those is only called in response to interactive human decision. I'm interested in problems with the second, though (but it only touches method trailer bytes, I'm not terribly concerned about it).
-C
On Mon, May 07, 2007 at 08:12:29PM -0700, Craig Latta wrote:
Those are part of Spoon. The first of those is only called inresponse to interactive human decision. I'm interested in problems with the second, though (but it only touches method trailer bytes, I'm not terribly concerned about it).
If you adopt the changes I did for the base VMMaker, then use the methods in ObjectMemory category "oop comparison" to do the comparisons, i.e. use #oop:isLessThan: rather than #<. This will do the type casting without upsetting the slang inliner.
BTW did any of you try the slang browser at http://wiki.squeak.org/squeak/5916? It's a lot more convenient than generating a new interp.c every time you change a method.
Dave
On May 7, 2007, at 6:35 PM, Bert Freudenberg wrote:
On May 7, 2007, at 17:25 , John M McIntosh wrote:
Ok, I was working with Craig a bit on this and I noticed for example.
Mmmm actually
if (((longAt(foo->freeBlock)) & AllButTypeMask) > foo-
shrinkThreshold) {
/* begin shrinkObjectMemory: */
One has to be careful if somehow longAt(foo->freeBlock) could be > 2GB, say running a 3GB image and messing with a 2GB chunk of memory. Well not sure if you have a free block > 2GB mmm
Gee I wonder.... After a full GC, mmmmm
-- ======================================================================== === John M. McIntosh johnmci@smalltalkconsulting.com Corporate Smalltalk Consulting Ltd. http://www.smalltalkconsulting.com ======================================================================== ===
On Mon, May 07, 2007 at 09:35:26PM -0400, Bert Freudenberg wrote:
On May 7, 2007, at 17:25 , John M McIntosh wrote:
Ok, I was working with Craig a bit on this and I noticed for example.
usqInt endOfMemory;
sqInt fwdBlock2;
/* begin restoreHeadersAfterForwardBecome: */ fwdBlock2 = ((foo->endOfMemory + BaseHeaderSize) + 7) & (WordMask - 7); flag("Dan"); fwdBlock2 += BytesPerWord * 4;Really shouldn't one consider that if a unsigned value gets assigned to a signed integer one should really consider that a possible problem?
Needless to say I believe the code above won't work as intended.
Shooting from the hip and all, but I'd say thanks to two's complement this actually should work. Far from pretty or desirable, but it should generate the same code (unless the C compiler inserts specific checks which it should not and AFAIK it does not).
Right. Twos compliment arithetic does the right thing, and as near as I can tell all oop addition and subtraction works correctly even if the variables are declared sqInt.
Dave
On Wed, Apr 25, 2007 at 06:59:49AM -0400, David T. Lewis wrote:
The attached zip contains six change sets and an update for sqMemoryAccess.h. The changes are intended to resolve problems with oop variable declarations and comparison operations that may occur on platforms that assign object memory to high virtual memory address values.
Can anyone confirm whether these changes are producing the intended results, i.e. a system that used to crash with >2GB oop issues, and no longer crashes after applying the changes? I don't have any Squeak platform that exhibits the problem, so so I can't confirm whether or not the problem is actually resolved.
Thanks,
Dave
Ok, well I did turn unsigned/signed warnings on and did wander thru the entire set of messge looking for something wrong, but did not find anything. Right now I need to alter the mac VM source code to handle sqInt values versus integer, then compile up a test harness that will allocate memory over or saddling the 2GB boundary. Maybe start this later today or tomorrow if time permits...
On May 27, 2007, at 6:22 AM, David T. Lewis wrote:
On Wed, Apr 25, 2007 at 06:59:49AM -0400, David T. Lewis wrote:
The attached zip contains six change sets and an update for sqMemoryAccess.h. The changes are intended to resolve problems with oop variable declarations and comparison operations that may occur on platforms that assign object memory to high virtual memory address values.
Can anyone confirm whether these changes are producing the intended results, i.e. a system that used to crash with >2GB oop issues, and no longer crashes after applying the changes? I don't have any Squeak platform that exhibits the problem, so so I can't confirm whether or not the problem is actually resolved.
Thanks,
Dave
-- ======================================================================== === John M. McIntosh johnmci@smalltalkconsulting.com Corporate Smalltalk Consulting Ltd. http://www.smalltalkconsulting.com ======================================================================== ===
Ok, I build a new VM using these changes then poked about and wondered about the following compares as below.
Also did some tests, by altering mmap I ran the VM at the 3GB boundary, that seemed to work ok.
I then attempted to run it at the 1.5 GB boundary using a memory size of
641,732,608 at the 1.5GB boundary
The basic image (a 3.5 image) started at 20MB. I then allocated 450 mb of memory and did macro-benchmarks
| suck | suck := OrderedCollection new. suck add: (ByteArray new: 1024*1024*450). 100 timesRepeat: [ Smalltalk macroBenchmarks. suck add: (ByteArray new: 1024*1024*1). Transcript show: Smalltalk garbageCollectMost;cr. Transcript show: Smalltalk garbageCollect;cr].
However when young space approached, when over, inbetween, whatever the 2GB boundary the VM core dumped. I was unable to printAllStacks(). I'll look a bit further and put some more debugging in to see if I can determine why.
- questionable? signed/unsigned compares at least from the compiler viewpoint.
primitivePerformAt: lookupClass primitiveDoPrimitiveWithArgs
self success: (self stackPointerIndex + arraySize) < cntxSize].
primitiveObjectAtPut primitiveObjectAt
self success: index <= ((self literalCountOf: thisReceiver) + LiteralStart).
postGCAction (self sizeOfFree: freeBlock) > shrinkThreshold Ok here if the size of the free > 2GB that is an issue Also shrinkThreshold is signed, but should it be?
incrementalGC (((self sizeOfFree: freeBlock) < growHeadroom)
I think you need to check usage of sizeOfFree: it could be > 2GB, very unlikely, but one never knows
On May 27, 2007, at 6:22 AM, David T. Lewis wrote:
On Wed, Apr 25, 2007 at 06:59:49AM -0400, David T. Lewis wrote:
The attached zip contains six change sets and an update for sqMemoryAccess.h. The changes are intended to resolve problems with oop variable declarations and comparison operations that may occur on platforms that assign object memory to high virtual memory address values.
Can anyone confirm whether these changes are producing the intended results, i.e. a system that used to crash with >2GB oop issues, and no longer crashes after applying the changes? I don't have any Squeak platform that exhibits the problem, so so I can't confirm whether or not the problem is actually resolved.
Thanks,
Dave
-- ======================================================================== === John M. McIntosh johnmci@smalltalkconsulting.com Corporate Smalltalk Consulting Ltd. http://www.smalltalkconsulting.com ======================================================================== ===
On 8-Jun-07, at 2:07 PM, John M McIntosh wrote:
- questionable? signed/unsigned compares at least from the compiler
viewpoint.
primitivePerformAt: lookupClass primitiveDoPrimitiveWithArgs
self success: (self stackPointerIndex + arraySize) < cntxSize].
That shouldn't be a problem; stackPointerIndex is pretty much limited to 32 words, arraySize is practically limited to quite small but could possibly get large in insane code and cntxSize is 40 (-ish?) words
primitiveObjectAtPut primitiveObjectAt
self success: index <= ((self literalCountOf: thisReceiver) +LiteralStart).
again, operating inside methods/context/etc so fairly limited value range. literal count is limited to 256 I think, LiteralStart is 6 or thereabouts.
postGCAction (self sizeOfFree: freeBlock) > shrinkThreshold
Looks like sizeOfFree returns a signed int. shrinkThreshold should only be a +ve number anyway and should be checked somewhere in the setting of its value. Guess sizeOfFree ought to be changed.
tim -- tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Useful random insult:- Can't find log base two of 65536 without a calculator.
On Fri, Jun 08, 2007 at 02:07:35PM -0700, John M McIntosh wrote:
Ok, I build a new VM using these changes then poked about and wondered about the following compares as below.
Also did some tests, by altering mmap I ran the VM at the 3GB boundary, that seemed to work ok.
I then attempted to run it at the 1.5 GB boundary using a memory size of
641,732,608 at the 1.5GB boundary
The basic image (a 3.5 image) started at 20MB. I then allocated 450 mb of memory and did macro-benchmarks
| suck | suck := OrderedCollection new. suck add: (ByteArray new: 1024*1024*450). 100 timesRepeat: [ Smalltalk macroBenchmarks. suck add: (ByteArray new: 1024*1024*1). Transcript show: Smalltalk garbageCollectMost;cr. Transcript show: Smalltalk garbageCollect;cr].
However when young space approached, when over, inbetween, whatever the 2GB boundary the VM core dumped.
Hi John,
Thanks for the feedback. It's good that you've got a fairly repeatable failure scenario. When I did those changes, I was working on a 64 bit machine, and the Squeak heap gets allocated at e.g. 0x2b39e0c5a000, so I would not have been seeing the 2GB boundary on that machine. I guess I should go back to a 32 bit box and reproduce the problem there.
I might not get a chance to look at this further for a couple of weeks though. Right now my pet project is to see if I can get Areithfa Ffenestri working on unix.
Dave
David, in order to test this I changed
debug = mmap( 2147483648U-512*1024*1024, gMaxHeapSize+pageSize
then asked for 600MB of squeak memory. mmap allocated memory for me on the 2GB-512MB boundary, so I then had 88 MB of space over the 2GB boundary. I could work towards.
I'd think if you look at sqUnixMemory.c and fiddle a bit with
if (MAP_FAILED == (heap= mmap(0, heapLimit, MAP_PROT, MAP_FLAGS, devZero, 0)))
you might be able to change the 0 to say.. .
0x7FFFFFFFF0000000
Then ask for 600MB of memory for the image.
That would set the end of memory at 0x8000000015800000 344MB over the negative sign boundary.
Check heap then, it *might* say 0x7FFFFFFFF0000000 if it works as expected. I was able to use this to set the start at 1gb, 1.5gb, 2gb, 3gb on the mac in 32bit mode.
On Jun 9, 2007, at 8:08 AM, David T. Lewis wrote:
Hi John,
Thanks for the feedback. It's good that you've got a fairly repeatable failure scenario. When I did those changes, I was working on a 64 bit machine, and the Squeak heap gets allocated at e.g. 0x2b39e0c5a000, so I would not have been seeing the 2GB boundary on that machine. I guess I should go back to a 32 bit box and reproduce the problem there.
I might not get a chance to look at this further for a couple of weeks though. Right now my pet project is to see if I can get Areithfa Ffenestri working on unix.
Dave
-- ======================================================================== === John M. McIntosh johnmci@smalltalkconsulting.com Corporate Smalltalk Consulting Ltd. http://www.smalltalkconsulting.com ======================================================================== ===
Attached is a change set that includes changes to work with size of free block as usqInt and the fix for the incCompMove: do loop.
With this fix it appears the VM will run below and cross the 2GB boundary in 32bit, and run at the 3GB boundary in 32bit mode.
On Jun 9, 2007, at 8:08 AM, David T. Lewis wrote:
Thanks for the feedback. It's good that you've got a fairly repeatable failure scenario.
-- ======================================================================== === John M. McIntosh johnmci@smalltalkconsulting.com Corporate Smalltalk Consulting Ltd. http://www.smalltalkconsulting.com ======================================================================== ===
ok, in debugging vm I get failure in
static inline sqInt longAtput(sqInt oop, sqInt val) { return longAtPointerput(pointerForOop(oop), val); }
oop is 0xe02e2c98 val is 310,038
called via inCompMove
} else { newFreeChunk = newOop + (sizeBitsOf(newOop)); /* begin setSizeOfFree:to: */ longAtput(newFreeChunk, (bytesFreed & AllButTypeMask) | HeaderTypeFree); }
newOop is 0x7ffb4558 newFreeChunk is -533844840 or 0xe02e2c98 Well that has a wrong feel to it... Let me attached an image of the debugger vars in case someone wants to puzzle out the issue.
On May 27, 2007, at 6:22 AM, David T. Lewis wrote:
On Wed, Apr 25, 2007 at 06:59:49AM -0400, David T. Lewis wrote:
The attached zip contains six change sets and an update for sqMemoryAccess.h. The changes are intended to resolve problems with oop variable declarations and comparison operations that may occur on platforms that assign object memory to high virtual memory address values.
Can anyone confirm whether these changes are producing the intended results, i.e. a system that used to crash with >2GB oop issues, and no longer crashes after applying the changes? I don't have any Squeak platform that exhibits the problem, so so I can't confirm whether or not the problem is actually resolved.
Thanks,
Dave
-- ======================================================================== === John M. McIntosh johnmci@smalltalkconsulting.com Corporate Smalltalk Consulting Ltd. http://www.smalltalkconsulting.com ======================================================================== ===
Further testing at the 2gb boundary shows a variety of different errors from run to run. I'd say the incremental GC logic trash memory somehow as it goes over the 2gb boundary.
On May 27, 2007, at 6:22 AM, David T. Lewis wrote:
On Wed, Apr 25, 2007 at 06:59:49AM -0400, David T. Lewis wrote:
The attached zip contains six change sets and an update for sqMemoryAccess.h. The changes are intended to resolve problems with oop variable declarations and comparison operations that may occur on platforms that assign object memory to high virtual memory address values.
Can anyone confirm whether these changes are producing the intended results, i.e. a system that used to crash with >2GB oop issues, and no longer crashes after applying the changes? I don't have any Squeak platform that exhibits the problem, so so I can't confirm whether or not the problem is actually resolved.
Thanks,
Dave
-- ======================================================================== === John M. McIntosh johnmci@smalltalkconsulting.com Corporate Smalltalk Consulting Ltd. http://www.smalltalkconsulting.com ======================================================================== ===
I stuck some compares in the pointer accessors and got
for (w = firstWord; w <= lastWord; w += BytesPerWord) { longAtput(target, longAt(w)); target += BytesPerWord; }
in incCompMove
w is 0x86401004 firstWord is 0x7ffffff4 lastWord is 0x7ffffffc
mmm Oh wait w being an integer of value -2042621948 is well always less than lastWord.
Perhaps other suspects?
for (i = firstField; i <= lastField; i += BytesPerWord) { for (i = BaseHeaderSize; i <= lastField; i += BytesPerWord) { for (i = ((lastPointerOf(oop)) + BytesPerWord); i <= (sz - BaseHeaderSize); i += BytesPerWord) {
On May 27, 2007, at 6:22 AM, David T. Lewis wrote:
On Wed, Apr 25, 2007 at 06:59:49AM -0400, David T. Lewis wrote:
The attached zip contains six change sets and an update for sqMemoryAccess.h. The changes are intended to resolve problems with oop variable declarations and comparison operations that may occur on platforms that assign object memory to high virtual memory address values.
Can anyone confirm whether these changes are producing the intended results, i.e. a system that used to crash with >2GB oop issues, and no longer crashes after applying the changes? I don't have any Squeak platform that exhibits the problem, so so I can't confirm whether or not the problem is actually resolved.
Thanks,
Dave
-- ======================================================================== === John M. McIntosh johnmci@smalltalkconsulting.com Corporate Smalltalk Consulting Ltd. http://www.smalltalkconsulting.com ======================================================================== ===
BTW, this is a really great catch John. We have seen various lockups in this precise loop and the subtlety of the lockup condition is beautiful. I mean you really have to appreciate that the condition here is that "lastWord + BytesPerWord <= lastWord" for the situation to go wrong.
Cheers, - Andreas
John M McIntosh wrote:
I stuck some compares in the pointer accessors and got
for (w = firstWord; w <= lastWord; w += BytesPerWord) { longAtput(target, longAt(w)); target += BytesPerWord; }in incCompMove
w is 0x86401004 firstWord is 0x7ffffff4 lastWord is 0x7ffffffc
mmm Oh wait w being an integer of value -2042621948 is well always less than lastWord.
Perhaps other suspects?
for (i = firstField; i <= lastField; i += BytesPerWord) { for (i = BaseHeaderSize; i <= lastField; i += BytesPerWord) { for (i = ((lastPointerOf(oop)) + BytesPerWord); i <= (sz -BaseHeaderSize); i += BytesPerWord) {
On May 27, 2007, at 6:22 AM, David T. Lewis wrote:
On Wed, Apr 25, 2007 at 06:59:49AM -0400, David T. Lewis wrote:
The attached zip contains six change sets and an update for sqMemoryAccess.h. The changes are intended to resolve problems with oop variable declarations and comparison operations that may occur on platforms that assign object memory to high virtual memory address values.
Can anyone confirm whether these changes are producing the intended results, i.e. a system that used to crash with >2GB oop issues, and no longer crashes after applying the changes? I don't have any Squeak platform that exhibits the problem, so so I can't confirm whether or not the problem is actually resolved.
Thanks,
Dave
--
John M. McIntosh johnmci@smalltalkconsulting.com Corporate Smalltalk Consulting Ltd. http://www.smalltalkconsulting.com ===========================================================================
On Tue, Apr 24, 2007 at 07:29:39PM -0700, Andreas Raab wrote:
Wow. Nice work. I can't speak for the others but since we have the problem very practically at Qwaq I'd be willing to test drive these changes for a while and see if they fix the problems. The best way to do this would probably be to post change sets to this list (this allows me to go over them method by method more easily).
I posted the change sets under the subject line "VM patches for oop comparison and usqInt declarations".
Thanks for all the work!
Thanks for reviewing and test driving them. I hope they solve the problem.
Dave
Hi David -
Looking over your changes I see two consistent patterns: One is to change all oops from sqInt to usqInt and the other one is to use the "special" unsigned comparison for pointers. Is my interpretation essentially correct?
A related issue: It bothers me greatly how complex all of this stuff has become. The whole 32/64bit conversion (oopForPointer: etc) and now pointer comparisons (no longer using < or >) makes me wonder of how sustainable this is. Even I can't recall all the rules that have to be followed to write clean 32/64/2GB barrier code. I wish we had a way of getting back to a set of simpler rules... any ideas anyone? The one idea that I can think of immediately would be to support types in slang better and have a specific slang compiler which can (for example) catch signed/unsigned comparisons when you write them.
I'm open for any suggestions on how to improve this situation.
Cheers, - Andreas
David T. Lewis wrote:
On Tue, Apr 24, 2007 at 07:29:39PM -0700, Andreas Raab wrote:
Wow. Nice work. I can't speak for the others but since we have the problem very practically at Qwaq I'd be willing to test drive these changes for a while and see if they fix the problems. The best way to do this would probably be to post change sets to this list (this allows me to go over them method by method more easily).
I posted the change sets under the subject line "VM patches for oop comparison and usqInt declarations".
Thanks for all the work!
Thanks for reviewing and test driving them. I hope they solve the problem.
Dave
On 25-Apr-07, at 9:48 AM, Andreas Raab wrote:
A related issue: It bothers me greatly how complex all of this stuff has become. The whole 32/64bit conversion (oopForPointer: etc) and now pointer comparisons (no longer using < or >) makes me wonder of how sustainable this is. Even I can't recall all the rules that have to be followed to write clean 32/64/2GB barrier code. I wish we had a way of getting back to a set of simpler rules... any ideas anyone?
It's partly because of having the slang code runnable in the simulator. All the oops are integers and so we obviously have to convert them in the generated code if we want pointers. Would using some of the CPointer etc classes help? It would mean changing the simulator a bit to cope of course but if it means we can generate better VM code it is probably worth it. It's a long time since the basci code was written and it may be time to attempt a substantial rewrite. Machines have changed a lot in 10 years. Perhaps the at- cache isn't valuable anymore?
I have a feeling (not looked seriously at the code in ages, so excuse vagueness) that some of the complexity also stems from the work to support mixed 32 & 64 bitness. You can't just cast when you need to add/subtract otBase or whatever. If this facility is not needed then there may well be some simplifications possible.
tim -- tim Rowledge; tim@rowledge.org; http://www.rowledge.org/tim Attitudes are contagious. Mine might kill you
On Wed, Apr 25, 2007 at 09:48:55AM -0700, Andreas Raab wrote:
Hi David -
Looking over your changes I see two consistent patterns: One is to change all oops from sqInt to usqInt and the other one is to use the "special" unsigned comparison for pointers. Is my interpretation essentially correct?
Yes, exactly so.
There are only two real issues that I could see:
1) oop comparisons need to be done with unsigned values.
2) unwanted sign extension in pointerForOop() when the machine address size is 8 bytes and object memory word size is 4 bytes.
All oop arithmetic (e.g. adding to an oop) works fine regardless of signed or unsigned declarations. As an aside, whomever came up with the idea of twos compliment arithmetic deserves great credit for Doing Things Right In The First Place.
I thought that adding the four "special" comparison methods was somewhat less ugly than putting the casts into each impacted method throughout ObjectMemory and Interpreter. It also allowed me to add some method comments to explain why it was being done.
A related issue: It bothers me greatly how complex all of this stuff has become. The whole 32/64bit conversion (oopForPointer: etc) and now pointer comparisons (no longer using < or >) makes me wonder of how sustainable this is. Even I can't recall all the rules that have to be followed to write clean 32/64/2GB barrier code. I wish we had a way of getting back to a set of simpler rules... any ideas anyone? The one idea that I can think of immediately would be to support types in slang better and have a specific slang compiler which can (for example) catch signed/unsigned comparisons when you write them.
In my opinion, it is entirely sustainable. Assuming that my changes actually do something useful (which remains to be seen), I can say that it really did not take a great deal of time to do this. What did take some time is understanding the issues and figuring out how to deal with them without breaking the slang inliner. Putting these changes in place serves to document my understanding of the problem (again, assuming that I actually did understand it). In principle that should improve maintainability.
I'm open for any suggestions on how to improve this situation.
Two ideas:
1) If we could declare the return type of a method and still generate inlined code, then it would probably be possible to eliminate most of the type casting, and therefore also eliminate the four "special" comparison methods that implement the type casting.
2) It may be possible to write the conversion macros (oopForPointer: etc) in slang in such a way that they would be inlined in the generated code. If so, that would make the conversions considerably less opaque.
Dave
David T. Lewis wrote:
On Wed, Apr 25, 2007 at 09:48:55AM -0700, Andreas Raab wrote:
Looking over your changes I see two consistent patterns: One is to change all oops from sqInt to usqInt and the other one is to use the "special" unsigned comparison for pointers. Is my interpretation essentially correct?
Yes, exactly so.
Thanks (just trying to make sure that I'm getting the gist of the changes).
All oop arithmetic (e.g. adding to an oop) works fine regardless of signed or unsigned declarations. As an aside, whomever came up with the idea of twos compliment arithmetic deserves great credit for Doing Things Right In The First Place.
Indeed. It's pretty amazing to see how this stuff works regardless ;-)
I thought that adding the four "special" comparison methods was somewhat less ugly than putting the casts into each impacted method throughout ObjectMemory and Interpreter. It also allowed me to add some method comments to explain why it was being done.
Yes, I agree. I far prefer it to sprinkling the casts all over the places; it is a reminder about the fact that one needs to use those methods.
I'm open for any suggestions on how to improve this situation.
Two ideas:
- If we could declare the return type of a method and still generate
inlined code, then it would probably be possible to eliminate most of the type casting, and therefore also eliminate the four "special" comparison methods that implement the type casting.
Yes, that's a good idea and should be entirely doable. Really, all we need is a conversion of #returnType: into a cCoerce: in the inliner. Should be quite doable.
Cheers, - Andreas
vm-dev@lists.squeakfoundation.org
-
Andreas Raab -
Bert Freudenberg -
Craig Latta -
David T. Lewis -
Diego Gomez Deck -
John M McIntosh -
tim Rowledge