Skip to main content

Code review: PP/PBP code diffs for CDC WinCE build

47 replies [Last post]
Anonymous

The following submission from Davy Preuveneers is being code reviewed as
a commit to the repository:

https://phoneme.dev.java.net/issues/show_bug.cgi?id=25

Please let me know if anyone has any comments or questions during its
code review process.

Thanks,

Hinkmond

---------------------------------------------------------------------
To unsubscribe, e-mail: advanced-unsubscribe@phoneme.dev.java.net
For additional commands, e-mail: advanced-help@phoneme.dev.java.net

Reply viewing options

Select your preferred way to display the comments and click "Save settings" to activate your changes.
Hinkmond Wong

Hinkmond Wong wrote:

> Hi Davy,
>
> If you don't mind, I'm going to pare down the code changes in the branch
> with your code submission to just the PP for WinMobile related changes
> in the AWT parts of the code space, which is the heart of your
> submission and needed by a bunch of other developers, so very much
> appreciated:
>
> src/share/personal/classes/awt/peer_based/sun/awt/pocketpc/*.java
> src/share/personal/native/awt/pocketpc/PPC*.cpp
>
>
> And, then address the build errors and suggest new fixes for those
> separately, making sure your main part of your submission can build
> cleanly.
>
> Let me know if there is any problem with this approach.

Latest changes for Davy P's code submission for the PP on WinMobile port
are available as an SVN WebRev at:

https://j2me-cdc.dev.java.net/code-reviews/phoneME/davyp-pp-winmobile-20...

Please let me know if there are any comments on this code.

If there are no further comments, I will test and merge the changes back
from the branch to the trunk by Wed. 1/23/2008.

Thanks,

Hinkmond

---------------------------------------------------------------------
To unsubscribe, e-mail: advanced-unsubscribe@phoneme.dev.java.net
For additional commands, e-mail: advanced-help@phoneme.dev.java.net

xyzzy (Dean)

defs_personal_pocketpc.mk is still referencing

src/win32/javavm/include/wcejpeg_md.h

What is the change in PPCToolkit.cpp doing?

Hinkmond Wong wrote:
> Hinkmond Wong wrote:
>
>> Hi Davy,
>>
>> If you don't mind, I'm going to pare down the code changes in the
>> branch with your code submission to just the PP for WinMobile related
>> changes in the AWT parts of the code space, which is the heart of your
>> submission and needed by a bunch of other developers, so very much
>> appreciated:
>>
>> src/share/personal/classes/awt/peer_based/sun/awt/pocketpc/*.java
>> src/share/personal/native/awt/pocketpc/PPC*.cpp
>>
>>
>> And, then address the build errors and suggest new fixes for those
>> separately, making sure your main part of your submission can build
>> cleanly.
>>
>> Let me know if there is any problem with this approach.
>
>
> Latest changes for Davy P's code submission for the PP on WinMobile port
> are available as an SVN WebRev at:
>
> https://j2me-cdc.dev.java.net/code-reviews/phoneME/davyp-pp-winmobile-20...
>
>
> Please let me know if there are any comments on this code.
>
>
> If there are no further comments, I will test and merge the changes back
> from the branch to the trunk by Wed. 1/23/2008.
>
>
> Thanks,
>
> Hinkmond
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: advanced-unsubscribe@phoneme.dev.java.net
> For additional commands, e-mail: advanced-help@phoneme.dev.java.net
>

---------------------------------------------------------------------
To unsubscribe, e-mail: advanced-unsubscribe@phoneme.dev.java.net
For additional commands, e-mail: advanced-help@phoneme.dev.java.net

Hinkmond Wong

https://j2me-cdc.dev.java.net/code-reviews/phoneME/davyp-pp-winmobile-20...

xyzzy (Dean) wrote:
> defs_personal_pocketpc.mk is still referencing
>
> src/win32/javavm/include/wcejpeg_md.h

OK. I just fixed that on the branch.

> What is the change in PPCToolkit.cpp doing?

Davy, here are the 2 lines you changed in PPCToolkit.cpp:

*+ if (env->GetJavaVM(&JVM) != 0)*
*+ return;

*Was that some debugging/troubleshooting lines you had in that file? It looks like those 2 lines are not needed. Is that correct?

Thanks,

Hinkmond

[att1.html]

davyp
Offline
Joined: 2007-01-03

> Davy, here are the 2 lines you changed in
> PPCToolkit.cpp:
>
> *+ if (env->GetJavaVM(&JVM) != 0)*
> *+ return;
>
>
> *Was that some debugging/troubleshooting lines you
> had in that file? It looks like those 2 lines are
> not needed. Is that correct?

I added those two lines, because the initIDs method in QtToolkit.cc has
them as well. I used the Qt implementation as a reference to see what
needed to be added for the WinCE implementation. I guess it is safe to
remove these two lines.

Davy

Hinkmond Wong

phonemeadvanced@mobileandembedded.org wrote:
>> Davy, here are the 2 lines you changed in
>> PPCToolkit.cpp:
>>
>> *+ if (env->GetJavaVM(&JVM) != 0)*
>> *+ return;
>>
>>
>> *Was that some debugging/troubleshooting lines you
>> had in that file? It looks like those 2 lines are
>> not needed. Is that correct?
>>
>
> I added those two lines, because the initIDs method in QtToolkit.cc has
> them as well. I used the Qt implementation as a reference to see what
> needed to be added for the WinCE implementation. I guess it is safe to
> remove these two lines.
>
Hi Davy,

OK. I just removed those 2 lines from the branch also.

Thanks!

Hinkmond

[att1.html]

Hinkmond Wong

phonemeadvanced@mobileandembedded.org wrote:
> The jpeg related headers are indeed new. Feel free to move my jmorecfg.h somewhere else if you do not like it at that place. The pragma makes sure that the original jmorecfg.h header file (located in src/share/basis/native/image/jpeg/lib/) is again reused if another source file includes jmorecfg.h. Consider the pragma as a temporary override of the original jmorecfg.h. I do not redirect to a file under src/win32/basis, but I guess you mean the directory above?
>
> The wceutil.h file was added as a quick fix to resolve the compilation of personal\native\compat\wcecompat.c. That one includes "wceutil.h". As such, the source file presumes the header file exists in the same directory as the c file (cfr. the include of "resource.h" that does exist in the same directory). I therefore created it again. However, there is a wceutil.h header file located in the parent directory of the c file. Hence, you could also fix this by changing wcecompat.c to include "../wceutil.h" instead of "wceutil.h".
>
> I had a similar build problem with share\personal\native\awt\pocketpc\ppcerror.h. It also includes wceutil.h but not with double quotes. In this case, we could an extra include flag to the compiler's options, if we want to avoid adding another wceutil.h

Hi Davy,

If you don't mind, I'm going to pare down the code changes in the branch
with your code submission to just the PP for WinMobile related changes
in the AWT parts of the code space, which is the heart of your
submission and needed by a bunch of other developers, so very much
appreciated:

src/share/personal/classes/awt/peer_based/sun/awt/pocketpc/*.java
src/share/personal/native/awt/pocketpc/PPC*.cpp

And, then address the build errors and suggest new fixes for those
separately, making sure your main part of your submission can build cleanly.

Let me know if there is any problem with this approach.

Thanks,

Hinkmond

---------------------------------------------------------------------
To unsubscribe, e-mail: advanced-unsubscribe@phoneme.dev.java.net
For additional commands, e-mail: advanced-help@phoneme.dev.java.net

davyp
Offline
Joined: 2007-01-03

Hi Hinkmond,

That is fine. After these patches have been applied, I can test what already works and what does not. Hopefully, it will already help others to build the sources too and see whether they come across some of the remaining issues I had (e.g. the scopetest thingy) or if these issues are just due to my build setup.

Thanks,
Davy

cjplummer
Offline
Joined: 2006-10-16

For RobotHelper.java, it is small enough that you could just create a unique version for each port. The another way is to use refelection to create the RobotHelper subclass instance. The basis version already does this, although it is still hard coding the name QtRobotHelper. I think there is a peerset property somewhere that would contain the peerset string, such as Qt, Gtk, or PPC. Hinkmond would know. If there isn't, you could add a property that contains the peerset name. Grep the makefile for SYSTEM_PROPERTY to see how to add one. It would have to be a private namespace name, like com.sun.awt.peerset.

For the jmorecfg.h problem, I can't believe someone thought adding a type called "boolean" would be a good idea, but since we got this jpeg code from JavaSE, and want to be able to continue to resync with it, I guess we ware stuck with "boolean" until JavaSE cleans it up. However, I think there is a problem with the webrev Hinkmond produced, because it doesn't show any diffs in these files, only that they were copied (and I don't believe they were actually copied).

Chris

Hinkmond Wong

phonemeadvanced@mobileandembedded.org wrote:
> ...
>
> However, I think there is a problem with the webrev Hinkmond produced, because it doesn't show any diffs in these files, only that they were copied (and I don't believe they were actually copied)

I believe the files that are marked as copies are ones that were newly
added by Davy.

See:
https://j2me-cdc.dev.java.net/code-reviews/phoneME/davyp-pp-winmobile-20...
(See files that have the note "(was https:...)")

Davy, please confirm.

They show up as "copied" because I merged from my branch to my working
copy trunk in order to produce the WebRev. So, to the WebRev tool it
seems like I "copied" the files from my branch to the trunk instead of
what I really did which was to add the files according to Davy's diffs
to my branch.

Is that correct, Davy?

Thanks,

Hinkmond

---------------------------------------------------------------------
To unsubscribe, e-mail: advanced-unsubscribe@phoneme.dev.java.net
For additional commands, e-mail: advanced-help@phoneme.dev.java.net

cjplummer
Offline
Joined: 2006-10-16

> I believe the files that are marked as copies are
> ones that were newly
> added by Davy.
>

Let me ask another question then. Why are all these new header files needed? Some seem to be a workaround for the boolean problem. However, I have concerns that a filed called src/win32/javavm/include/win32/jmorecfg.h is redirecting the #include to a file under src/win32/basis. Dean should probably comment more on this also.

Also, I have concerns about the proliferation of wceUtil.h. We already have two. Now we are adding two more. Why?

Chris

davyp
Offline
Joined: 2007-01-03

The jpeg related headers are indeed new. Feel free to move my jmorecfg.h somewhere else if you do not like it at that place. The pragma makes sure that the original jmorecfg.h header file (located in src/share/basis/native/image/jpeg/lib/) is again reused if another source file includes jmorecfg.h. Consider the pragma as a temporary override of the original jmorecfg.h. I do not redirect to a file under src/win32/basis, but I guess you mean the directory above?

The wceutil.h file was added as a quick fix to resolve the compilation of personal\native\compat\wcecompat.c. That one includes "wceutil.h". As such, the source file presumes the header file exists in the same directory as the c file (cfr. the include of "resource.h" that does exist in the same directory). I therefore created it again. However, there is a wceutil.h header file located in the parent directory of the c file. Hence, you could also fix this by changing wcecompat.c to include "../wceutil.h" instead of "wceutil.h".

I had a similar build problem with share\personal\native\awt\pocketpc\ppcerror.h. It also includes wceutil.h but not with double quotes. In this case, we could an extra include flag to the compiler's options, if we want to avoid adding another wceutil.h.

Davy

xyzzy
Offline
Joined: 2006-08-30

I think we should look into how Java SE solves this jpeg boolean problem.
Are they defining HAVE_BOOLEAN somewhere?

Hinkmond Wong

phonemeadvanced@mobileandembedded.org wrote:
> Just two small remarks:
>
> One of the patches that is currently applied in this branch cannot be applied to the main trunk as such. It modifies sun/awt/RobotHelper.java in order to call PPCRobotHelper instead of the (hardcoded) QtRobotHelper. I can imagine a proper solution is needed if multiple GUI backends (like GTK) will be supported at the same time in the same tree.
>
> In the patches, I also defined a DllMain method in the PPCToolkit.cpp file to initialize the pocketpcawt.dll library. However, if CVM_PRELOAD_LIB is set to true, you get a linking problem due to two DllMain methods being defined, one in the PPCToolkit.cpp file and the other in net_util_md.c. There is another file (Setup.cpp) that defines a DllMain entry point for a DLL, but that file does not seem to be compiled according to my logs.
>

Thanks, Davy! Looks like fisheye4 is still not caught up with the
latest rev of the phoneME repository, so I'm going to make a manual
WebRev instead of depending on fisheye4 to give us a easy way to see the
code diffs for this change.

I'll let everyone know when that is ready so that we can make changes
(from above) to adjust the code submission.

Thanks,

Hinkmond

---------------------------------------------------------------------
To unsubscribe, e-mail: advanced-unsubscribe@phoneme.dev.java.net
For additional commands, e-mail: advanced-help@phoneme.dev.java.net

Hinkmond Wong

Hinkmond Wong wrote:
> phonemeadvanced@mobileandembedded.org wrote:
>> Just two small remarks:
>>
>> One of the patches that is currently applied in this branch cannot be
>> applied to the main trunk as such. It modifies
>> sun/awt/RobotHelper.java in order to call PPCRobotHelper instead of
>> the (hardcoded) QtRobotHelper. I can imagine a proper solution is
>> needed if multiple GUI backends (like GTK) will be supported at the
>> same time in the same tree.
>>
>> In the patches, I also defined a DllMain method in the PPCToolkit.cpp
>> file to initialize the pocketpcawt.dll library. However, if
>> CVM_PRELOAD_LIB is set to true, you get a linking problem due to two
>> DllMain methods being defined, one in the PPCToolkit.cpp file and the
>> other in net_util_md.c. There is another file (Setup.cpp) that
>> defines a DllMain entry point for a DLL, but that file does not seem
>> to be compiled according to my logs.
>>
>
> Thanks, Davy! Looks like fisheye4 is still not caught up with the
> latest rev of the phoneME repository, so I'm going to make a manual
> WebRev instead of depending on fisheye4 to give us a easy way to see
> the code diffs for this change.
>
> I'll let everyone know when that is ready so that we can make changes
> (from above) to adjust the code submission.

Code Review WebRev for Davy's contribution is now available at:

https://j2me-cdc.dev.java.net/code-reviews/phoneME/davyp-pp-winmobile-20...

Please take a look at the code diffs and let me know by Tue. 15Dec2007
if you have any suggested changes or corrections. If not, I'll go ahead
and merge the changes back to the trunk.

I'll be testing building the port to see what new build flags and other
build instructions are needed.

Thanks,

Hinkmond

---------------------------------------------------------------------
To unsubscribe, e-mail: advanced-unsubscribe@phoneme.dev.java.net
For additional commands, e-mail: advanced-help@phoneme.dev.java.net

Chris

I think Davy said you need to remove the RobotHelper.java changes
because they break linux/QT builds.

In defs_personal_pocketpc.mk, I'd still like to see the SUFFIX
references replaced with the already existing POSTFIX macros so this
doesn't get propagated any further.

What is the reason for not having writeStandardIO return an int anymore.
I don't recall seeing this mentioned.

Why are there are bunch of files being listed as having moved. I think
many of them don't have any corresponding changes, like wcejpeg_md.h and
jmorecfg.h.

Chris

Hinkmond Wong wrote:
>
> Code Review WebRev for Davy's contribution is now available at:
>
> https://j2me-cdc.dev.java.net/code-reviews/phoneME/davyp-pp-winmobile-20...
>
>
>
> Please take a look at the code diffs and let me know by Tue. 15Dec2007
> if you have any suggested changes or corrections. If not, I'll go
> ahead and merge the changes back to the trunk.
>
> I'll be testing building the port to see what new build flags and
> other build instructions are needed.
>
> Thanks,
>
> Hinkmond
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: advanced-unsubscribe@phoneme.dev.java.net
> For additional commands, e-mail: advanced-help@phoneme.dev.java.net
>

---------------------------------------------------------------------
To unsubscribe, e-mail: advanced-unsubscribe@phoneme.dev.java.net
For additional commands, e-mail: advanced-help@phoneme.dev.java.net

davyp
Offline
Joined: 2007-01-03

> I think Davy said you need to remove the RobotHelper.java changes
> because they break linux/QT builds.

Yes, if you want to build for linux/Qt, you need to use QtRobotHelper. If you target Windows Mobile use PPCRobotHelper. Leave this patch out for the time being until a proper solution is found if we are going to support multiple GUI backends in the same tree. Instead of having Qt hardcoded, would generating a file at compile time be a solution to use the right GUI backend?

> In defs_personal_pocketpc.mk, I'd still like to see the SUFFIX
> references replaced with the already existing POSTFIX macros so this
> doesn't get propagated any further.

I already replaced those references in my local tree.

> What is the reason for not having writeStandardIO return an int anymore.
> I don't recall seeing this mentioned.

I probably had a build problem. I am currently testing the latest stable build (b50) with the same patches applied, and it seems to compile without this one. I will do some more testing later on.

> Why are there are bunch of files being listed as having moved. I think
> many of them don't have any corresponding changes, like wcejpeg_md.h and
> jmorecfg.h.

The changes to cdc/src/win32/javavm/include/wcejpeg_md.h and cdc/src/win32/javavm/include/win32/jmorecfg.h were needed because I had an issue with the definition of the 'boolean' type. If you leave these patches out I get:

z:\project\phoneme\cdc\src\share\basis\native\image\jpeg\lib\jmorecfg.h(263) : error C2371: 'boolean' : redefinition; different basic types
c:\program files\windows ce tools\wce420\pocket pc 2003\include\armv4\rpcndr.h(161) : see declaration of 'boolean'

This issue was mentioned before on this forum:

http://forums.java.net/jive/thread.jspa?threadID=21524&tstart=100

I first modified jmorecfg.h directly to see if the boolean type already existed,but that was not considered a good way to solve the problem.
That is why I defined a pragma to use a different jmorecfg.h when building for WinCE.

> Chris

Davy

davyp
Offline
Joined: 2007-01-03

>> What is the reason for not having writeStandardIO
>> return an int anymore.
>> I don't recall seeing this mentioned.
>
> I probably had a build problem. I am currently
> testing the latest stable build (b50) with the same
> patches applied, and it seems to compile without this
> one. I will do some more testing later on.

I had another look at the writeStandardIO because it has been a while since modified that file.

A grep on the CDC sources tells me that this method is refered to in:
src/win32/personal/native/console/wceConsole.c
=> returns int
src/win32/javavm/include/io_md.h
=> returns void
src/win32/javavm/runtime/wceIOWrite.c
=> returns void
src/win32/javavm/runtime/io_md.c
=> called without using the return value

That is why I changed the "returning int" to "returning void". If the method should return an int, then the win32/javavm/include/io_md.h header needs to be modified as well.

Davy

cjplummer
Offline
Joined: 2006-10-16

You are getting the jio_sprintf error because you are not linking with $(CVM_IMPL_LIB).

In order to fix the build order, you should add $(CVM_IMPL_LIB) to the dependency list when building $(JPEG_LIB_PATHNAME).

As for the various CVM_PRELOAD_LIB settings, what matters more is the CVM_STATICLINK_LIBS settings, which defaults to be the same as CVM_PRELOAD_LIB. Thus when CVM_PRELOAD_LIB=true, there is no libjpeg.so, since the objects are linked with the cvm executable.

The wiki build instructions you referenced that say to use CVM_PREOLOAD_LIB=true are because currently CVM_PRELOAD_LIB=false doesn't work for wm5 builds that include optional JSRs (and maybe MIDP is a problem also). Actually the real problem is that CVM_STATICLINK_LIBS=false does not work, so you can use CVM_PREOLOAD_LIB=false CVM_STATICLINK_LIBS=true.

The one combination of flags that I think don't makes sense to use and therefore does not need to be supported is CVM_PREOLOAD_LIB=true CVM_STATICLINK_LIBS=false. All the dlls will get loaded on launch anyway because of reference from romized classes, so they might as well be statically linked with the cvm executable.

Chris

xyzzy
Offline
Joined: 2006-08-30

I'd actually like to see someone fix the ROMizer so that it supports lazy binding to native
methods. That means adding a level of indirection or making method blocks
writable.

Dean

hinkmond
Offline
Joined: 2003-12-01

I've committed Davy's subset of changes to the branch:

http://fisheye4.cenqua.com/changelog/phoneme/?cs=8490

Please take a look at the changeset (it's easier to look at the diffs this way) and let me know if anyone has any final comments before I merge to the trunk.

Thanks,

Hinkmond

Chris

The URL is not working for me. It says:

No changesets with that ID found.

Chris

phonemeadvanced@mobileandembedded.org wrote:
> I've committed Davy's subset of changes to the branch:
>
> http://fisheye4.cenqua.com/changelog/phoneme/?cs=8490
>
> Please take a look at the changeset (it's easier to look at the diffs this way) and let me know if anyone has any final comments before I merge to the trunk.
>
>
> Thanks,
>
> Hinkmond
> [Message sent by forum member 'hinkmond' (hinkmond)]
>
> http://forums.java.net/jive/thread.jspa?messageID=249840
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: advanced-unsubscribe@phoneme.dev.java.net
> For additional commands, e-mail: advanced-help@phoneme.dev.java.net
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: advanced-unsubscribe@phoneme.dev.java.net
For additional commands, e-mail: advanced-help@phoneme.dev.java.net

Hinkmond Wong

> Chris wrote:
> The URL is not working for me. It says:
>
> No changesets with that ID found.

Hi Chris,

I'm seeing the same problem now. I was hoping that fisheye4 would be
faster than it currently is with keeping up with the phoneme repository.

http://fisheye4.cenqua.com/changelog/phoneme/

It's at rev 8488 right now. I'll keep watching for when it catches up
to my commit (rev. 8490) and will let everyone know when it's available.

Thanks,

Hinkmond

---------------------------------------------------------------------
To unsubscribe, e-mail: advanced-unsubscribe@phoneme.dev.java.net
For additional commands, e-mail: advanced-help@phoneme.dev.java.net

davyp
Offline
Joined: 2007-01-03

Just two small remarks:

One of the patches that is currently applied in this branch cannot be applied to the main trunk as such. It modifies sun/awt/RobotHelper.java in order to call PPCRobotHelper instead of the (hardcoded) QtRobotHelper. I can imagine a proper solution is needed if multiple GUI backends (like GTK) will be supported at the same time in the same tree.

In the patches, I also defined a DllMain method in the PPCToolkit.cpp file to initialize the pocketpcawt.dll library. However, if CVM_PRELOAD_LIB is set to true, you get a linking problem due to two DllMain methods being defined, one in the PPCToolkit.cpp file and the other in net_util_md.c. There is another file (Setup.cpp) that defines a DllMain entry point for a DLL, but that file does not seem to be compiled according to my logs.

Davy

Chris

I had no idea there was a DllMain in net_util_md.c. We should always
avoid DllMain or any other type of .dll or .so file for CDC stack
libraries, since this prevents things from working properly when static
linking all the native code.

The DllMain in net_util_md.c runs now as the DllMain for cvmi.dll.
However, I'm working on changes that will allow you to build with just a
cvm.exe and no cvmi.dll, at which time this code will never be called.
Maybe Dean can suggest a solution. In general I would suggest a static
initializer be used to trigger initializations rather than a DllMain.

There's also a DllMain in src/win32/personal/native/Setup.cpp that just
returns true. I'm not sure why it is even there. Lastly, there is
win32/personal/native/compat/wceCompat.c that is only enabled when
CVM_STATICLINK_LIBS=false. I'm not sure how things are suppose to work
when CVM_STATICLINK_LIBS=true, since "theInstance" never gets
initialized, but is later used by wceSetMenuBar, which just returns NULL
if theInstance == NULL.

Chris

phonemeadvanced@mobileandembedded.org wrote:
> Just two small remarks:
>
> One of the patches that is currently applied in this branch cannot be applied to the main trunk as such. It modifies sun/awt/RobotHelper.java in order to call PPCRobotHelper instead of the (hardcoded) QtRobotHelper. I can imagine a proper solution is needed if multiple GUI backends (like GTK) will be supported at the same time in the same tree.
>
> In the patches, I also defined a DllMain method in the PPCToolkit.cpp file to initialize the pocketpcawt.dll library. However, if CVM_PRELOAD_LIB is set to true, you get a linking problem due to two DllMain methods being defined, one in the PPCToolkit.cpp file and the other in net_util_md.c. There is another file (Setup.cpp) that defines a DllMain entry point for a DLL, but that file does not seem to be compiled according to my logs.
>
> Davy
> [Message sent by forum member 'davyp' (davyp)]
>
> http://forums.java.net/jive/thread.jspa?messageID=250018
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: advanced-unsubscribe@phoneme.dev.java.net
> For additional commands, e-mail: advanced-help@phoneme.dev.java.net
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: advanced-unsubscribe@phoneme.dev.java.net
For additional commands, e-mail: advanced-help@phoneme.dev.java.net

xyzzy
Offline
Joined: 2006-08-30

The DllMain in net_util_md.c seems to have been inherited from Java SE in 1997.
We should clean this up, now that sio_init_WSA() does the same thing.

Chris

So we can just yank the DllMain in net_util_md.c, right?

Chris

phonemeadvanced@mobileandembedded.org wrote:
> The DllMain in net_util_md.c seems to have been inherited from Java SE in 1997.
> We should clean this up, now that sio_init_WSA() does the same thing.
> [Message sent by forum member 'xyzzy' (xyzzy)]
>
> http://forums.java.net/jive/thread.jspa?messageID=250044
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: advanced-unsubscribe@phoneme.dev.java.net
> For additional commands, e-mail: advanced-help@phoneme.dev.java.net
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: advanced-unsubscribe@phoneme.dev.java.net
For additional commands, e-mail: advanced-help@phoneme.dev.java.net

xyzzy
Offline
Joined: 2006-08-30

Maybe. Is sio_init_WSA() always called? Is it called
early enough?

> So we can just yank the DllMain in net_util_md.c,
> right?
>
> Chris
>
> phonemeadvanced@mobileandembedded.org wrote:
> > The DllMain in net_util_md.c seems to have been
> inherited from Java SE in 1997.
> > We should clean this up, now that sio_init_WSA()
> does the same thing.
> > [Message sent by forum member 'xyzzy' (xyzzy)]
> >
> >
> http://forums.java.net/jive/thread.jspa?messageID=2500
> 44
> >
> >
> ------------------------------------------------------
> ---------------
> > To unsubscribe, e-mail:
> advanced-unsubscribe@phoneme.dev.java.net
> > For additional commands, e-mail:
> advanced-help@phoneme.dev.java.net
> >
> >
>
> ------------------------------------------------------
> ---------------
> To unsubscribe, e-mail:
> advanced-unsubscribe@phoneme.dev.java.net
> For additional commands, e-mail:
> advanced-help@phoneme.dev.java.net

cjplummer
Offline
Joined: 2006-10-16

It's called from CVMinitVMTargetGlobalState.

> Maybe. Is sio_init_WSA() always called? Is it
> called
> early enough?
>
> > So we can just yank the DllMain in net_util_md.c,
> > right?
> >
> > Chris
> >

xyzzy
Offline
Joined: 2006-08-30

OK, but one requests winsock version 1.1, and the other requests
2.2. If we remove DllMain then we may need to change
SIO to request 2.2

> It's called from CVMinitVMTargetGlobalState.
>
> > Maybe. Is sio_init_WSA() always called? Is it
> > called
> > early enough?
> >
> > > So we can just yank the DllMain in
> net_util_md.c,
> > > right?
> > >
> > > Chris
> > >

cjplummer
Offline
Joined: 2006-10-16

We also have CVMnetStartup, which calls WSAStartup with version 2.0 for wince, and 1.1 for win32. It is called form CVMinitStaticState, which is called early on by JNI_CreateJavaVM. So currently there are 3 separate calls to WSAStartup at startup time, each of which requests a different version.

The SIOinit version is called a bit later by CVMinitVMTargetGlobalState, which is called by CVMinitVMGlobalState, which is called JNI_CreateJavaVM long after it calls CVMinitStaticState. So this would indicate that the SIOinit version is not needed, so I wonder why it was ever added.

What is the consequence of multiple calls requesting different versions? Why does win32 request the much older 1.1 version.

Chris

> OK, but one requests winsock version 1.1, and the
> other requests
> 2.2. If we remove DllMain then we may need to
> change
> SIO to request 2.2
>
> > It's called from CVMinitVMTargetGlobalState.
> >
> > > Maybe. Is sio_init_WSA() always called? Is it
> > > called
> > > early enough?
> > >
> > > > So we can just yank the DllMain in
> > net_util_md.c,
> > > > right?
> > > >
> > > > Chris
> > > >

Dean Long

WSAStartup is described here:

http://msdn2.microsoft.com/en-us/library/ms742213.aspx

You should ask the SIO author why the init function was added.
Win32 probably asks for 1.1 because we inherited it from Java SE
and never changed it. We should determine which version our
libraries require.

Dean

phonemeadvanced@mobileandembedded.org wrote:
> We also have CVMnetStartup, which calls WSAStartup with version 2.0 for wince, and 1.1 for win32. It is called form CVMinitStaticState, which is called early on by JNI_CreateJavaVM. So currently there are 3 separate calls to WSAStartup at startup time, each of which requests a different version.
>
> The SIOinit version is called a bit later by CVMinitVMTargetGlobalState, which is called by CVMinitVMGlobalState, which is called JNI_CreateJavaVM long after it calls CVMinitStaticState. So this would indicate that the SIOinit version is not needed, so I wonder why it was ever added.
>
> What is the consequence of multiple calls requesting different versions? Why does win32 request the much older 1.1 version.
>
> Chris
>
>> OK, but one requests winsock version 1.1, and the
>> other requests
>> 2.2. If we remove DllMain then we may need to
>> change
>> SIO to request 2.2
>>
>>> It's called from CVMinitVMTargetGlobalState.
>>>
>>>> Maybe. Is sio_init_WSA() always called? Is it
>>>> called
>>>> early enough?
>>>>
>>>>> So we can just yank the DllMain in
>>> net_util_md.c,
>>>>> right?
>>>>>
>>>>> Chris
>>>>>
> [Message sent by forum member 'cjplummer' (cjplummer)]
>
> http://forums.java.net/jive/thread.jspa?messageID=250218
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: advanced-unsubscribe@phoneme.dev.java.net
> For additional commands, e-mail: advanced-help@phoneme.dev.java.net
>

---------------------------------------------------------------------
To unsubscribe, e-mail: advanced-unsubscribe@phoneme.dev.java.net
For additional commands, e-mail: advanced-help@phoneme.dev.java.net

xyzzy
Offline
Joined: 2006-08-30

The fake jio_snprintf in src/share/basis/native/image/jpeg/lib/jerror.c looks wrong.
I don't understand the INSIDE_JAVAI changes either.

Dean

Terrence Barr - Evangelist, Java Mobile & Embedded

Excellent! From what I'm seeing in the Java ME community this
is really representative of the growing interest in phoneME
and right in line with with a number of contributions expected
over the next couple of months.

Thanks Davy for pursuing this!

-- Terrence

phonemeadvanced@mobileandembedded.org wrote:
> There were no comments for this code review, so I will be committing this code into the trunk. Thanks very much to Davy Preuveneers our first open source code contributor for the phoneME Advanced project.
>
>
> Hinkmond
> [Message sent by forum member 'hinkmond' (hinkmond)]
>
> http://forums.java.net/jive/thread.jspa?messageID=249577
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: advanced-unsubscribe@phoneme.dev.java.net
> For additional commands, e-mail: advanced-help@phoneme.dev.java.net
>
[terrence.barr.vcf]
---------------------------------------------------------------------
To unsubscribe, e-mail: advanced-unsubscribe@phoneme.dev.java.net
For additional commands, e-mail: advanced-help@phoneme.dev.java.net

Roger Brinkley

Great job Davy...

Binky

Terrence Barr - Evangelist, Java Mobile & Embedded wrote:
> Excellent! From what I'm seeing in the Java ME community this
> is really representative of the growing interest in phoneME
> and right in line with with a number of contributions expected
> over the next couple of months.
>
> Thanks Davy for pursuing this!
>
> -- Terrence
>
> phonemeadvanced@mobileandembedded.org wrote:
>> There were no comments for this code review, so I will be committing
>> this code into the trunk. Thanks very much to Davy Preuveneers our
>> first open source code contributor for the phoneME Advanced project.
>>
>>
>> Hinkmond
>> [Message sent by forum member 'hinkmond' (hinkmond)]
>>
>> http://forums.java.net/jive/thread.jspa?messageID=249577
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: advanced-unsubscribe@phoneme.dev.java.net
>> For additional commands, e-mail: advanced-help@phoneme.dev.java.net
>>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: advanced-unsubscribe@phoneme.dev.java.net
> For additional commands, e-mail: advanced-help@phoneme.dev.java.net
>

---------------------------------------------------------------------
To unsubscribe, e-mail: advanced-unsubscribe@phoneme.dev.java.net
For additional commands, e-mail: advanced-help@phoneme.dev.java.net

cjplummer
Offline
Joined: 2006-10-16

Why is scopetest.b.C1 being removed from the build?

Why aren't LIB_POSTFIX and LIB_LINK_POSTFIX being used rather than adding the SUFFIX macros.

Chris

davyp
Offline
Joined: 2007-01-03

> Why is scopetest.b.C1 being removed from the build?

Do not ask me why, but I get the following error without the patch:

(cd ../../build/win32-arm-ppc03/./personal_classes; /cygdrive/c/j2sdk1.4.2_14/bin/jar cf z:/project/phoneme/cdc/build/win32-arm-ppc03/lib/personal.jar *)
Checking for test classes to compile ...
make: *** No rule to make target `../../build/win32-arm-ppc03/./testclasses/scopetest/b/C1.class', needed by `../../build/win32-arm-ppc03/./.testclasses.plist'. Stop.

I have been looking into why I get this error and only with this particular class. Although with the patch applied the scopetest.b.C1 class still gets compiled, it would indeed be much better if I could pinpoint the exact cause of this problem. I have not been succesful thus far.

> Why aren't LIB_POSTFIX and LIB_LINK_POSTFIX being
> used rather than adding the SUFFIX macros.

I am not quite sure, but I think the SUFFIX macros were used before they were renamed to POSTFIX? There were some leftovers of the SUFFIX macros in that file to compile the wcecompat library. As the SUFFIX macros were undifined, I redefined them.

> Chris

Davy

cjplummer
Offline
Joined: 2006-10-16

> > Why is scopetest.b.C1 being removed from the
> build?
>
> Do not ask me why, but I get the following error
> without the patch:
>
> (cd ../../build/win32-arm-ppc03/./personal_classes;
> /cygdrive/c/j2sdk1.4.2_14/bin/jar cf
> z:/project/phoneme/cdc/build/win32-arm-ppc03/lib/perso
> nal.jar *)
> Checking for test classes to compile ...
> make: *** No rule to make target
> `../../build/win32-arm-ppc03/./testclasses/scopetest/b
> /C1.class', needed by
> `../../build/win32-arm-ppc03/./.testclasses.plist'.
> Stop.
>
> I have been looking into why I get this error and
> only with this particular class. Although with the
> patch applied the scopetest.b.C1 class still gets
> compiled, it would indeed be much better if I could
> pinpoint the exact cause of this problem. I have not
> been succesful thus far.
>

Here's a commit message for a change I made on 5/23/06 that added scopetest:

-Added scopetest.a.C2, scopetest.b.C1, and
cvmtests.TypeidRefcountHelper to the list Test classes to compile. For
some reason javac doesn't compile classes that are instantiated, but
never referenced otherwise. This fixes a buildl problem with
CVM_PRELOAD_TEST=true builds.

So apparently these are needed to avoid a javac problem, although I'm pretty sure it was just a specific version of javac that gave us this problem. For this reason I'd rather you figured out why the build failed instead of removing scopetest.b.C1. If we can reproduce tha problem after your commit (without this change), then probably someone on our end can solve it. Otherwise you'll need to investigate some more.

> > Why aren't LIB_POSTFIX and LIB_LINK_POSTFIX being
> > used rather than adding the SUFFIX macros.
>
> I am not quite sure, but I think the SUFFIX macros
> were used before they were renamed to POSTFIX? There
> were some leftovers of the SUFFIX macros in that file
> to compile the wcecompat library. As the SUFFIX
> macros were undifined, I redefined them.
>
I think it would be best to switch to the existing POSTFIX macros.

Chris

Hinkmond Wong

phonemeadvanced@mobileandembedded.org wrote:
>>> Why is scopetest.b.C1 being removed
>>>
...
>>> I have been looking into why I get this error and
>>> only with this particular class. Although with the
>>> patch applied the scopetest.b.C1 class still gets
>>> compiled, it would indeed be much better if I could
>>> pinpoint the exact cause of this problem. I have not
>>> been succesful thus far.
>>>
>>>
>
> Here's a commit message for a change I made on 5/23/06 that added scopetest:
>
> -Added scopetest.a.C2, scopetest.b.C1, and
> cvmtests.TypeidRefcountHelper to the list Test classes to compile. For
> some reason javac doesn't compile classes that are instantiated, but
> never referenced otherwise. This fixes a buildl problem with
> CVM_PRELOAD_TEST=true builds.
>
> So apparently these are needed to avoid a javac problem, although I'm pretty sure it was just a specific version of javac that gave us this problem. For this reason I'd rather you figured out why the build failed instead of removing scopetest.b.C1. If we can reproduce tha problem after your commit (without this change), then probably someone on our end can solve it. Otherwise you'll need to investigate some more.
>
>
>
>>> Why aren't LIB_POSTFIX and LIB_LINK_POSTFIX being
>>> used rather than adding the SUFFIX macros.
>>>
>> I am not quite sure, but I think the SUFFIX macros
>> were used before they were renamed to POSTFIX? There
>> were some leftovers of the SUFFIX macros in that file
>> to compile the wcecompat library. As the SUFFIX
>> macros were undifined, I redefined them.
>>
>>
> I think it would be best to switch to the existing POSTFIX macros.
>
> Chris
>

Hi Davy,

I'll propose this:

- I'll commit the diffs for your AwtPPCstubs, AwtPPC, Compiler, and Jpeg
(minus jerror.c code that Dean (xyzzy) pointed out) to a branch and
leave out the ScopeTest, Link, and Find changes for now.
- Then generate a WebRev of the above subset of your diffs for others to
view for now for the final code review.
- We can work out any separate build issues you might still see with
this branch, since the remaining build issues may be specific to your
build environment.

Let me know if there is any problem with this approach. Otherwise, I'll
go ahead and make this branch so that it is easy for everyone to see
what we are discussing for finalizing the code.

Thanks,

Hinkmond

---------------------------------------------------------------------
To unsubscribe, e-mail: advanced-unsubscribe@phoneme.dev.java.net
For additional commands, e-mail: advanced-help@phoneme.dev.java.net

davyp
Offline
Joined: 2007-01-03

That is fine by me.

Regarding the Link*.diff:

The reason why I removed $(CVM_IMPL_LIB) in that line was because the cvmi.lib library is built after the jpeg lib.

Linking ../../build/win32-arm-ppc03/./bin/awtjpeg.dll
LINK : fatal error LNK1181: cannot open input file './bin/cvmi.lib'

According to my logs, wcecompat.dll is built first, then pocketpcawt.dll, then awtjpeg.dll and only then cvmi.dll

The $(CVM_IMPL_LIB) was not there many revisions ago. Perhaps this linking issue can be better resolved by changing the order in which the cvm and jpeg libraries are built. I think this also answers to some extent cjplummer's question.

Regarding Dean's remark on jerror.c:
I get a linking error for jio_snprintf.

Creating library bin/awtjpeg.lib and object bin/awtjpeg.exp
jerror.o : error LNK2019: unresolved external symbol jio_snprintf referenced in function format_message
bin/awtjpeg.dll : fatal error LNK1120: 1 unresolved externals

If the above jpeg linking issue is resolved, then the jpeg library can also link against cvmi, and hence also against jio_snprintf implemented in javavm/runtime/utils.c

Davy

xyzzy
Offline
Joined: 2006-08-30

What happens with a CVM_PRELOAD_LIB=true build? Will cvmi.dll then depend on
awtjpeg.dll, creating a circular dependency? Or do we link the awtjpeg .obj files
into cvmi.dll? Anyone remember?

One way to break the dependency is to define jio_snprintf. Another way
is to create a cvmi.lib using the LIB tool before cvmi.dll is built:

http://msdn2.microsoft.com/en-us/library/f0z8kac4(VS.71).aspx

davyp
Offline
Joined: 2007-01-03

In that case the jpeg objects are all linked into cvmi.dll. With CVM_PRELOAD_LIB=true the jerror.c and jpeg link issues do not occur. Is enabling this option a recommendation? I noticed that in the Wiki http://wiki.java.net/bin/view/Mobileandembedded/PhoneMEAdvancedGSGWinMobile
this option is set as well.

Davy

hinkmond
Offline
Joined: 2003-12-01

There were no comments for this code review, so I will be committing this code into the trunk. Thanks very much to Davy Preuveneers our first open source code contributor for the phoneME Advanced project.

Hinkmond

xyzzy
Offline
Joined: 2006-08-30

I don't understand some of the changes. Did you review all the changes Hinkmond? If not who did?

Hinkmond Wong

phonemeadvanced@mobileandembedded.org wrote:
> I don't understand some of the changes. Did you review all the changes Hinkmond? If not who did?
>

Yes, I reviewed the changes. The important parts of the changes are for
stubbing the AWT classes for the WinMobile build. Do you have a
specific question on one of the changes?

I have a question to Davy about the change in Find.rev7334.diff why he
needed to specify "/usr/bin/find" instead of "find". If he has a
workaround for his environment, I plan not to incorporate that delta.

Let me know if you want to ask Davy any other questions. I've cc'd him,
but he is also on the advanced@phoneme.dev.java.net alias.

Thanks,

Hinkmond

---------------------------------------------------------------------
To unsubscribe, e-mail: advanced-unsubscribe@phoneme.dev.java.net
For additional commands, e-mail: advanced-help@phoneme.dev.java.net

davyp
Offline
Joined: 2007-01-03

> I have a question to Davy about the change in Find.rev7334.diff why he
> needed to specify "/usr/bin/find" instead of "find". If he has a
> workaround for his environment, I plan not to incorporate that delta.

I have a particular remote building setup where I call my own build script
remotely through ssh. In that case the PATH variable is not set correctly
(the cygwin bin paths are appended instead of prepended). As such, the
phoneme build scripts mistakenly use the Windows version of the find
program (instead of the cygwin version). Feel free to ignore the patch. I will
modify my own build scripts instead.

Davy

Hinkmond Wong

phonemeadvanced@mobileandembedded.org wrote:
>> I have a question to Davy about the change in Find.rev7334.diff why he
>> needed to specify "/usr/bin/find" instead of "find". If he has a
>> workaround for his environment, I plan not to incorporate that delta.
>
> I have a particular remote building setup where I call my own build script
> remotely through ssh. In that case the PATH variable is not set correctly
> (the cygwin bin paths are appended instead of prepended). As such, the
> phoneme build scripts mistakenly use the Windows version of the find
> program (instead of the cygwin version). Feel free to ignore the patch. I will
> modify my own build scripts instead.

Hi Davy,

Sounds good. I'll remove the Find.rev7334.diff from the code submission.

One more question about your Link.*.diff: Is there a library in the
build variable you removed that was causing an error? Is there
something specific to the WinMobile build that requires that change.

Thanks,

Hinkmond

---------------------------------------------------------------------
To unsubscribe, e-mail: advanced-unsubscribe@phoneme.dev.java.net
For additional commands, e-mail: advanced-help@phoneme.dev.java.net

cjplummer
Offline
Joined: 2006-10-16

>
> One more question about your Link.*.diff: Is there a library in the
> build variable you removed that was causing an error? Is there
> something specific to the WinMobile build that requires that change.
>

I will be commiting a change soon that will make it so $(CVM_IMPL_LIB) is always part of the $(SO_LINK_CMD) on win32. So if this is a problem, I need to know.

Also, if we want builds to work on win32/x86, the SO_LINK_CMD can't have arguments after it. This is because win32-x86-vc8 (and vc6) add to the SO_LINK_CMD to run mt.exe, so if you do the following:

$(SO_LINK_CMD) $(CVM_IMPL_LIB) $(JPEG_LIB_LIBS)

$(CVM_IMPL_LIB) $(JPEG_LIB_LIBS) end up getting passed to mt.exe rather than the link command. The fix is to make SO_LINK_CMD take an argument, so you call it like this:

$(call SO_LINK_CMD, $(CVM_IMPL_LIB) $(JPEG_LIB_LIBS))

This change I also have in my workspace and will commit sometime in the near future.

Chris