RE: Code Review

[David Griswold]

Hi Andras,

 

Thanks very much for your work so far.  Now that there is a released binary under VS6, I am ready to move to a later release; sorry for the slow movement.   I would like to try to avoid doing a VS7.1 release, since many of us (including me) don't have it and anyway it isn't the latest version.  Although it doesn't run yet under VS8, I think we should focus on getting that working now since that is what most of us have to work with, and it is the latest version. 

 

I will commit your two bug fixes into the repository as soon as I can build using them under VS8.  Since you have changed the .asm files, we can't just use the existing .obj files, so how are you getting the .obj files generated by TASM converted to COFF so they link correctly?

 

As for the inline friend function issue: It would be very desirable for us to keep those friend functions as inline functions, since they may be performance critical operations.  Are you saying that it is simply not possible to have them as inline functions anymore?  I haven't committed those changes yet, since I want to understand exactly why we can't have them as inline functions.

 

Any progress getting it to run under VS8?

 

Thanks,

-Dave

-----Original Message-----
From: András Páhi [mailto:pa...@info-m.hu]
Sent: Monday, October 02, 2006 9:42 PM
To: David Griswold
Subject: RE: Code Review

Hi David,

 

No, you understand everything to get the sources to compile under

VS6. Gilad Bracha pointed out, that it would be good to port the sources

to more recent versions like VS7.1 or VS8.

 

The changes you mentioned are only needed to compile under VS7.1.

The VS7.1 C++ compiler does different scoping for the friend functions

than the VS6 compiler did. So the change is effectively to move those

functions outside of the class declarations and thus removing their

inline properties.

 

I have sent two files for the original Strongtalk group at groups.yahoo.com.

The first one contained only the changes needed to compile under VS6

and some minor changes to compile under VS7.1. (strongtalk-sources.zip)

 

The other one strongtalk-source-1.0-vs71.zip which contains the entire

source, contains two important fixes, which are actually bugs in the original

code. The first one is in the VirtualSpaces::remove function. Inside the for

loop the condition is p->next = sp which is incorrect. p->next == sp is the

right code.

 

The second one is a calling convention mix-up. The primitives in vm/prims/

double_prims_asm.asm and smi_prims_asm.asm contain routines which

use the __stdcall calling convention. The corresponding .hpp files

(double_prims.hpp and smi_prims.hpp) miss the PRIM_API calling

convention declaration in the external function declarations. So does the

prim.cpp file in the prim_fntypeX declarations.

 

I have included these modifications in the attached files. If you want to run

the code through VS6 no other modifications are necessary.

 

I would like to stabilize first the VM code under some VisualStudio version,

which you prefer. Just chose one of VS6 or VS7.1, which you like. I had a

hard time to fire up the VM under VS8, but no results so far. The VM crashed

in the Debug and Product versions also, so I gave up and concentrated to

VS7.1 only. I did a quick trial to compile under a recent GCC version, but

it requires *LOT OF WORK*, which I think is overkill in this time. I would like

to get the VM stable enough for one compiler on one platform.

 

Regards,

Andras Pahi

-----Eredeti üzenet-----
Feladó: David Griswold [mailto:David.G...@acm.org]
Küldve: Monday, October 02, 2006 3:02 AM
Címzett: pa...@info-m.hu
Másolatot kap: strongtal...@googlegroups.com
Tárgy: Code Review

Hi Andras,

Sorry it's taken me so long to review the changes you submitted to get Strongtalk to build under VS6.   Here are my comments:

I don't understand why you have changed the inline function declarations into non-inline definitions.   We want those to be inline functions since they are performance critical, very small operations.  The system seems to compile and run with only the following changes, as far as I can see:

• compiler\expr.hpp 36: need (?:) to return bool, rather than implicit conversion.
• runtime\debug.cpp 41: bug: MATERIALIZE_BOOLEAN_FLAG should be MATERIALIZE_INTEGER_FLAG.  This is an error in the original code.
• topIncludes\types.hpp 36: change so that bool is not redefined for VC++.
• recompiler\recompiler.hpp 48:  needRecompilerCounter must return bool; no return type declared; didn't match def. This is an error in the original code.
• code\inliningdb.cpp 739: file_out_all is declared to return a bool, which is wrong and doesn't match the decl of int.  This is an error in the original code.

Is there something I am not understanding?

Cheers,

Dave

[Thiago Silva]

-----Original Message-----
From: András Páhi [mailto:pa...@info-m.hu]
Sent: Monday, October 02, 2006 9:42 PM
To: David Griswold
Subject: RE: Code Review

(...) 

I did a quick trial to compile under a recent GCC version, but

it requires *LOT OF WORK*, which I think is overkill in this time. I would like

to get the VM stable enough for one compiler on one platform.

 

Regards,

Andras Pahi

I've setup a MS Windows + VS6 environment this weekend and was able to build strongtalk from the sources using the headers that David sent to the list (incls.zip). Indeed, bin/Makefile uses tool/makedeps.exe, wich is not currently present and, curiously, I've found it today in a previous .zip download of strongtalk.

To make things more interesting (I needed a little of fun :), I've also installed the MingW/MSYS tools and created a simple Makefile, one that gmake could understand. So far, I was able to compile 67 source files with g++ 3.4.2 (~100 sources to go). Most errors were pretty straight forward (macro errors and C++ syntax). But things are demanding special attention now that I'm facing C++ sources with inline assembly code ;) altough it seems there aren't much of that around.

Thiago Silva

[David Griswold]

The makedeps.exe which appeared in the earlier .zip files is no longer there because it shoudn't have been there in the first place; it was inadvertently put there in the zip file from Sun.  We're not sure where it came from, but since there isn't a known license for it, I've yanked it out of the .zips; please don't use it.  We will need to find another one, or stop using makedeps altogether.

 

As for the inline assembly; I don't believe there is any actual inline assembly in the C++.  There *is* assembly code there, but it is written using pure C++ constructs to model an assembler.  The C++ code then uses it to generate most of the interpreter when it starts up.  When development stopped Robert Griesemer was in the process of converting all the external assembler into that format, in C++.  Over time, we should complete that process, and then we won't need Turbo Assembler anymore.

-Dave

[Thiago Silva]

On 10/17/06, David Griswold <David.G...@acm.org> wrote:
>
>
> The makedeps.exe which appeared in the earlier .zip files is no longer there
> because it shoudn't have been there in the first place; it was inadvertently
> put there in the zip file from Sun. We're not sure where it came from, but
> since there isn't a known license for it, I've yanked it out of the .zips;
> please don't use it. We will need to find another one, or stop using
> makedeps altogether.

Ok, thanks for explain :)
Just out of curiosity, can anyone point out what exacly is the reason for using
those makedeps/generated includes? I mean, if my observation is correct,
makedeps creates the incls/* based on deps/includeDB, and this file is just an
specification of what sources include what. I'm not aware of scenarios where
such mechanism would help (instead of writing directly the dependencies in the
source files).

> As for the inline assembly; I don't believe there is any actual inline
> assembly in the C++.

I'm referring to this codes:

-prims/integerOps.cpp: lines 266, 287, 307, 330
-runtime/savedRegisters.cpp: line 65
-utilities/longInt.cpp:lines 43, 57, 71, 80
and so on...

> There *is* assembly code there, but it is written using
> pure C++ constructs to model an assembler. The C++ code then uses it to
> generate most of the interpreter when it starts up. When development stopped
> Robert Griesemer was in the process of converting all the external assembler
> into that format, in C++. Over time, we should complete that process, and
> then we won't need Turbo Assembler anymore.
> -Dave

Interesting.
I was wandering what faith those Tasm sources would have...

Thiago Silva

[David Griswold]

Hi Thiago,

Thiago Silva wrote:
> > As for the inline assembly; I don't believe there is any actual inline
> > assembly in the C++.
>
> I'm referring to this codes:
>
> -prims/integerOps.cpp: lines 266, 287, 307, 330
> -runtime/savedRegisters.cpp: line 65
> -utilities/longInt.cpp:lines 43, 57, 71, 80
> and so on...
>
> > There *is* assembly code there, but it is written using
> > pure C++ constructs to model an assembler. The C++ code then uses it to
> > generate most of the interpreter when it starts up. When
> development stopped
> > Robert Griesemer was in the process of converting all the
> external assembler
> > into that format, in C++. Over time, we should complete that
> process, and
> > then we won't need Turbo Assembler anymore.
> > -Dave

It appears you are right, there is a little bit of inline assembler in a few
places. Hopefully very little of it.

-Dave

[Thiago Silva]

Hello Carlo,

> I am trying to compile StrongTalk on MacOS X 10.4.8, also with g++ 3.4.
> I have two GNU makefiles.

I'm sorry I missed your previous email.
I didn't face the errors you did, so I'm not sure how to work around them...

> Can you send me a diff ?

I'm attaching the patch with modifications I've been done to compile
with MingW/MSYS (g++ 3.4.2) on MS Windows.

Some notes about the changes:

* I'm compiling with -DPRODUCT -DDELTA_COMPILER

* I didn't use -Dstd. Instead, I renamed "std" to Stdout and "err" to
Stderr (utilities/outputstream.?pp).

* Constructions like the following don't compile under g++:
char* name = f() ? "nmethod" : (isPIC() ? " PIC" : "count stub");

* There are lots of "for" loops declaring counters in it's own context
and then, using it latter, outside it's context (see
compiler/bitVector.cpp). I've been declaring such counters just before
the loop, so they are used in the entire method scope (there are
cleaner approaches for some cases, tough).

* "and", "or", "xor" identifiers are keywords. As for std, I've
renamed the identifiers with such names.

* I haven't tried using assembly Intel syntax. Maybe using it would be better...

* I didn't test too much some assembly blocks, even tough I pray for
it's rightness :)

> Also, which version of StrongTalk are you using ? Straight off SVN ?

Yep, SVN sources.

Thiago Silva