gcc -Wall -pedantic (was: sweep failed to find exact end of memory)

 
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:
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

(*) http://wiki.squeak.org/squeak/5916

 
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.

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:

 
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.

 
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 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; [hidden email]; 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:
>
> 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.

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

 
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 <[hidden email]>
Corporate Smalltalk Consulting Ltd.  http://www.smalltalkconsulting.com
========================================================================
===

 

On May 7, 2007, at 17:25 , John M McIntosh wrote:


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:

--
========================================================================
===
John M. McIntosh <[hidden email]>
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 <[hidden email]>
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

--
Craig Latta
improvisational musical informaticist
www.netjam.org
Smalltalkers do: [:it | All with: Class, (And love: it)]

 

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 <[hidden email]>
Corporate Smalltalk Consulting Ltd.  http://www.smalltalkconsulting.com
========================================================================
===

 
On Mon, May 07, 2007 at 09:35:26PM -0400, Bert Freudenberg wrote:
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