351246 - Load some of Mozilla into Highmem on OS/2

Status: RESOLVED FIXED

(SeaMonkey :: Build Config, enhancement)

Description

[Andy Willis (abwillis)]

Newer OS/2 kernels allow memory to be loaded into Highmem.  This patch adds the OBJ_ANY setting to DosAllocMem and DosAllocSharedMem calls, except for clipboard functions which causes pasted issues into command line sessions.  It also uses -Zhigh-mem which causes libc to use high memory for its heap.
There is one extra bit in the patch I am attaching... I have removed ordinal creation and for this preliminary patch I am not backing it out so I included the  
security/coreconf/OS2.mk patch to match the configure.in patch.
Patch to follow

Comment 1

[Andy Willis (abwillis)]

Attachment: High memory patch

Comment 2

[Andy Willis (abwillis)]

I've been building with these settings for a few months now.

Comment 3

[Peter Weilbacher]

Great, thanks Andy! Mine was almost the same and crashed but I am going to test yours for comparison.

Two questions:
- I guess you did the same changes to nsprpub/configure.in and directory/c-sdk/configure.in but forgot to diff them for the patch?
- Would you not have to add OBJ_ANY to DosGetNamedSharedMem() calls, too?

Comment 4

[Andy Willis (abwillis)]

I am not aware of the OBJ_ANY flag being used with DosGetNamedSharedMem().
Actually, I didn't even think about those configures :(
If this still doesn't work for you, then I would bet that the one in nsprpub is the one that causes the problem.
I need to build sunbird and firefox with the current settings but then I'll update those 2 configures and see what happens.

Comment 5

[Andy Willis (abwillis)]

I asked on #netlabs and it seems noone is using OBJ_ANY with the DosGetSharedMem.  Doodle says he also uses OBJ_ANY with DosAllocSharedMem but not with DosGetSharedMem. Yuri Dario put together a small test app and said that DosGetSharedMem indeed did not need it.
I completed a build where I added -Zhigh-mem to the other to configure.in files and I am using that build to post this.  I'll create another patch that includes those configure.in files.  It would be nice if RWS changes could get checked in so that creating patches would be easier :)

Comment 6

[Andy Willis (abwillis)]

Attachment: More complete

Here is the patch I meant to get out several days ago now.

Comment 7

[Walter Meinl]

(In reply to comment #6)
> Created an attachment (id=240867) [edit]
> More complete
> 
> Here is the patch I meant to get out several days ago now.
> 

I applied it and built successfully Firefox, Thunderbird and Xulrunner. However, running the programs I made three different experiences.
Firefox is rock-solid (built static, svg enabled).
Thunderbird was build shared because of lightning extension. Didn't start. When I reduced VIRTUALADDRESSLIMIT to 512 or copied the nspr libs from an earlier build back, it ran, suggesting that in case of shared it doesn't find the nspr libs somehow. The system load went up to 99% for 30 secs, a little bit down again for a second or two and then up again for several seconds to drop finally back on the value before I started. I rebuilt TB static (omitting lightning) and then it started, was in comparison to FF relatively unstable.
Xulrunner (with all the open patches applied): built shared. In this case I couldn't start simple by copying xulrunner-stub.exe to simple.exe. It ran however when I started it with xulrunner path-to-simple\application.ini. With the experience from the TB build I saved my old nspr-libs, but copying them over the highmem-nspr libs didn't change the behavior of xulrunner-stub.exe. The stub seems to have problems with the highmem, cause it works in a normal build as posted in the newsgroup

Comment 8

[Andy Willis (abwillis)]

I backed out the dosallocmem stuff and built xulrunner and it xulrunner-stub still did not work.  It must be the -Zhigh-mem as I had not backed it out.

Comment 9

[Andy Willis (abwillis)]

OK, I just did a build that includes the OBJ_ANY code but not the -Zhigh-mem and xulrunner-stub does now work.  I realize now that all my tests with -Zhigh-mem have been with static builds (other than xulrunner and it does work with the -Zhigh-mem when run as xulrunner path\application.ini).

Comment 10

[Walter Meinl]

Just to correct myself with regard to Thunderbird static with highmem. It's now also as stable as Firefox (after I found that the instability came from ft2lib.dll, new install resolved it). Thus, the patch works fine for statically linked applications.

Comment 11

[Andy Willis (abwillis)]

Attachment: hopefully complete

After debugging xulrunner-stub with Knut at Warpstock he found pretty quickly a couple of problems (and the resolutions) that would not allow it to work with -Zhigh-mem.  I haven't tested with a dynamic build yet but there is a good chance it is the same issue.  This patch is the same as the previous one except for the additions for the changes to os2io.c and nsLocalFileOS2.cpp.  I don't actually recall the specifics for os2io.c but it dealt with where some things were in the stack.  nsLocalFileOS2.cpp was using WinUpper which is apparently 16 bit and couldn't see the high stack.

Comment 12

[Walter Meinl]

(In reply to comment #11)
> Created an attachment (id=242599) [edit]
> hopefully complete
> 
> After debugging xulrunner-stub with Knut at Warpstock
Yeah, that rocks, Firefox on top of Xulrunner run by the stub, and thunderbird shared is also working. 

In the xpcom/io/nsLocalFileOS2.cpp part of the patch the last two lines are missing (only 8 lines), that are blank lines in the original file, simply to correct.

Comment 13

[Andy Willis (abwillis)]

Attachment: adds the missing lines for nslocalfileos2

Thanks for pointing that out, I would really like to see Rich's integration patches from bug 305061 checked in as it would make creating patches much, much easier.  I added the spaces to this patch.

Comment 14

[Peter Weilbacher]

strupr() is not in the ANSI standard as far as I can see, that could mean that we need a configure test for it. Does it take into account the process codepage at all? If not that might be an issue... Well, I am just compiling with this patch and will give it some testing.

Comment 15

[Andy Willis (abwillis)]

Knut said that strupr may not work with dbcs.  I am not sure about codepages.  The other option would be to wrap WinUpper the way that Rich did in nsRwsService.cpp.

Comment 16

[Peter Weilbacher]

The answer to my own question is no, "aäböÖo" becomes "AäBöÖO" with strupr() while it becomes "AÄBÖÖO" with WinUpper(). But actually, that is irrelevant, ext is local and in that context will just compare negative against the executable extensions anyway when it contains a non-ASCII char. But one should perhaps add a comment about this (maybe instead of the #if 0), something like

   // strupr() does not take into account non-ASCII characters but 
   // this is irrelevant for the possible extensions below

When testing this patch did you not notice any problems when starting programs from Mozilla (i.e. helper apps)? I haven't finished compiling but Jim Moe gave up on -Zhigh-mem because of that problem.

Comment 17

[Andy Willis (abwillis)]

I have not seen any issues with helper apps.  Maybe it is the helper apps involved though.  Any idea what helper apps he was using?

Comment 18

[Peter Weilbacher]

No, it was a general problem that executing other apps from within his program did not work with highmem. Hmm, in his posting he said that it was mainly about DosStartSession() -- of which there are a few instances found in the Mozilla code. The recommendation was to change those to DosExecProgram() instead...

About the hack in os2io.c: I think you should get Knut to tell you why allocating anew with alloca() helps and add another comment above it. (I have a hunch but might be wrong.) We should also keep in mind that this is another point that might break with compilers different from GCC. I am also not sure if in the libc06 (and libc05) implementations of alloca() the memory is automatically freed on return, documentation varies across implementations in that regard.

Well, my version runs at least. :-) I think we should all run our version with this patch included for a while to make sure that there are no extra issues before getting this in, as OS/2 users would really only notice half a year or more later and then it would be really hard to figure out where problems came from. Let me see if I can get this into the 2.x line of PmW-* apps. :-)

Comment 19

[Andy Willis (abwillis)]

I'll have to ask him again about the os2io.c, he was just on IRC earlier but got off before I could ask him about it.  I had built a debug version and he saw that something was trying to access the high stack that couldn't (it looks like it is the DosOpen just below the addition).  Memory address above 0x20000000 shows that high memory is being used.  I think this should work cross compiler, if highmem is not being used then the if will evaluate false and not be used and if it is true then even OW might would need it (not sure if OW has facility to use highmem or not, they may have a wrapper that makes it not necessary if they can use highmem as Knut is considering doing with WinUpper for instance- which is the reason for the #if 0 as then we could consider removing that code if the wrapper is done).
I agree with the idea of using it for awhile, I know sometimes I think a problem is with my setup and therefore don't take the time to track it down if it isn't urgent and later when I do track it down realize it was not my setup but a problem that has now been in the code for awhile.

Comment 20

[Andy Willis (abwillis)]

Comment on attachment 242659 [details] [diff] [review]
adds the missing lines for nslocalfileos2

Setting review flag per Mike's request.

Comment 21

[Andy Willis (abwillis)]

Peter one thought I just had... you have the tooltips working so that you can see a preview of a site by hovering over a tab as I understand it.  It always just shows a black box here.  If you build with these patches do tooltips still work for you?  Just to make sure that isn't my problem here.

Comment 22

[Peter Weilbacher]

It seems that this black-canvas problem also occurs for people in non-highmem builds (see Ilya's recent posts on the newsgroup).

Comment 23

[Peter Weilbacher]

Comment on attachment 242659 [details] [diff] [review]
adds the missing lines for nslocalfileos2

The patch also sets by-name linking using  FILTER='emxexp'. I understood from Knut's comments that with declspec this is not necessary any more, so I guess it should be removed.

Comment 24

[Peter Weilbacher]

Comment on attachment 242659 [details] [diff] [review]
adds the missing lines for nslocalfileos2

While trying to add the patch to my tree I notice that it is also missing to set -Zhigh-mem for NSS. The place to put that should be
http://lxr.mozilla.org/mozilla1.8/source/security/coreconf/OS2.mk#92

Comment 25

[Peter Weilbacher]

Comment on attachment 242659 [details] [diff] [review]
adds the missing lines for nslocalfileos2

Sorry to only comment bit by bit, I hope this is the last one...

The addition of -Zhigh-mem to the DSO_LDOPTS line under MOZ_OPTIMIZE causes it to be present twice on the command line (see nsprpub\config\autoconf.mk in the OBJDIR tree). I suspect that the addition to MOZ_OPTIMIZE_LDFLAGS is unnecessary, too.

Comment 26

[Peter Weilbacher]

OK, I have a non-static SeaMonkey from the 1.8 branch compiled now and it runs fine and also properly displays the tooltip canvas (at least on my machine). But how do I confirm that it actually uses high memory?

Comment 27

[Andy Willis (abwillis)]

Steven Levine says there is a way to check in Theseus but I didn't get a chance to have him show me how while at Warpstock.  Knut does it through the debugger.  It would be much easier to do through Theseus if I can figure out how to do it.  Guess I need to send Steven a note to get instructions.

Comment 28

[Peter Weilbacher]

Attachment: (patch used in 1.8 branch PmW version)

Just for reference, here is the patch that I used for the build of my enhanced SeaMonkey from the 1.8 branch. Because I didn't want to mess up my tree with only complicated ways to get it out again, I added --enable-os2-high-mem as an extra parameter. I do not suggest to get this in! The code changes are the same as suggested by Andy, but I added two more comments. The #warning bits and echos in configure.in are just to make it easier to check what I was building.

(I also use another extra paramter --enable-os2-link-by-name that I originally implemented for bug 331395 but that nobody wanted. It is included in this patch.)

Comment 29

[Andy Willis (abwillis)]

I got to thinking about it and added -Zhigh-mem to the mzfntcfg defines.mk.
DLLFLAGS = -O2 -Zomf -Zdll -Zmap -s -pipe \
	-Zlinker /EXEPACK:2 -Zlinker /PACKCODE -Zlinker /PACKCODE -Zhigh-mem
I think that is where it needs to go (file size definitely changed with it).
I can still play tetris and have text show up so it seems to work, though I still have to find how to verify it is loaded high.
There are some people that still run pre-Warp 4 fp13 systems.  The -Zhigh-mem is ignored on these but OBJ_ANY will probably fail.  There is one of 2 tests that could be run to test at runtime...  Use DosQuerySysState and if the Virtual address limit is greater than 512 then it can use OBJ_ANY or do the DosAllocMem (or shared) with OBJ_ANY and if it returns ERROR_INVALID_PARAMETER then make the call without OBJ_ANY.

Comment 30

[Andy Willis (abwillis)]

Attachment: obj_any won't work on pre-fp13 kernels

This patch checks the return code from the DosAllocMem and DosAllocSharedMem and if it failed tries again without obj_any before falling through.

Comment 31

[Peter Weilbacher]

Comment on attachment 243951 [details] [diff] [review]
obj_any won't work on pre-fp13 kernels

Another not-asked-for review by me. :-)

>-EXEFLAGS                += -Zlinker /DE
>+EXEFLAGS                += -Zlinker -Zhigh-mem /DE

I don't think this will work, /DE has to follow the -Zlinker parameter, right?

>+    APIRET rc = (DosAllocMem(&result, bytes, PAG_EXECUTE | PAG_READ |
>+    				    PAG_WRITE | PAG_COMMIT | OBJ_ANY);
>+                if (rc) rc = (DosAllocMem(&result, bytes, PAG_EXECUTE | PAG_READ |
>+    				    PAG_WRITE | PAG_COMMIT);

Something is wrong with the indentation here and in most other of the ifs. The tabs are not your fault, but I think they should go.

I think you should write it in a more compact way and be more explicit about the rc test. Using more curly brackets often helps to understand code more easily, but that is just a personal preference. Something like this

> APIRET rc = (DosAllocMem(&result, bytes, PAG_EXECUTE | PAG_READ |
>                                          PAG_WRITE | PAG_COMMIT | OBJ_ANY);
> if (rc != NO_ERROR) {
>   if ((DosAllocMem(&result, bytes, PAG_EXECUTE | PAG_READ |
>                                    PAG_WRITE | PAG_COMMIT)
>       != NO_ERROR) {
>     return 0;
>   }
> }

(I hope that the linebreaks don't get messed up.)


>-        FILTER='emxexp -o'
>+        FILTER='emxexp'

That is still in there...

>+    if ((ULONG)name >= 0x20000000)
>+    {
>+        size_t len = strlen(name) + 1;
>+        char *copy = (char *)alloca(len);
>+        memcpy(copy, name, len);
>+        name = copy;
>+    }

How about a comment? :-)


>-DLLFLAGS		= -DLL -OUT:$@ -MAP:$(@:.dll=.map)
>-EXEFLAGS    		= -PMTYPE:VIO -OUT:$@ -MAP:$(@:.exe=.map) -nologo -NOE
>+DLLFLAGS		= -DLL -OUT:$@ -MAP:$(@:.dll=.map) -Zhigh-mem
>+EXEFLAGS    		= -PMTYPE:VIO -OUT:$@ -MAP:$(@:.exe=.map) -nologo -NOE -Zhigh-mem
> OBJDIR_TAG 		= _OPT
>+LDFLAGS			+= -Zhigh-mem

Hey, interesting, I didn't spot that before. I didn't spot that before for EXEs. But are you sure that this won't add unnecessary multiple -Zhigh-mem parameters for DLLs?

Finally, are you sure that you need the patch to directory/c-sdk/config/OS2.mk at all? I think that the changes in directory/c-sdk/configure.in are good enough.

Comment 32

[Andy Willis (abwillis)]

Attachment: Cleaner pre-fp13 safe version.

It is possible it will have extra -Zhigh-mem but I double checked with Knut multiple times won't adversely affect its use and this way it should catch all uses of it in case it is needed. 
I removed the removal of the emxexp change, I think we should probably make that go away though.  
I cleaned up the readability and added some comments so that what was going on is clear.  I like this way with the if's as it makes it so that they all flow the same.

Comment 33

[Andy Willis (abwillis)]

Attachment: wrap dosstartsession calls

After reading that DosStartSession wasn't safe for highmem in the newsgroup I asked Knut and he suggested using <os2safe.h>.  This wraps some calls that are unsafe.  I used it for all the DosStartSession calls.  I also checked for other functions and found DosForceDelete that was questionable in os_os2.c.  It _may_ not be needed but it would be hard to track down for certain if it did so I went ahead and added it as it adds minimal overhead.  This _should_ allow helper apps to start now, won't have time to build until I get home.

Comment 34

[Andy Willis (abwillis)]

Attachment: corrected pre-fp13 fix with dosstartsession wrap

Ok, building I found that I had messed up while using clipview and messed up which clip I was using for the paste operation in several files.

Comment 35

[Andy Willis (abwillis)]

Attachment: same as the last but without silly syntax errors

Ok, this time I am posting this from a build with those patches (which this is why I generally wait till it has finished building before posting).

Comment 36

[Mike Kaply [:mkaply]]

Can you set the WinUpper to strupr changes with things like an a umlaut?

Also, the formatting needs to match the surrounding code in these files - "When in rome"...

Comment 37

[Peter Weilbacher]

(In reply to comment #33)
> After reading that DosStartSession wasn't safe for highmem in the newsgroup I
> asked Knut and he suggested using <os2safe.h>.  This wraps some calls that are
> unsafe.

Andy, can you say a few more words on this? I looked at that header file but all it does it re-define some OS/2 APIs to the same name prefixed with "Safe". Where are those SafeXXX functions implemented and how?

(In reply to comment #36)
> Can you set the WinUpper to strupr changes with things like an a umlaut?

Mike, umlauts are not important there, it's just for comparing to four possible file extensions (that don't have special characters).

Comment 38

[Andy Willis (abwillis)]

The safe calls wrap the unsafe calls so that they are passed safe parameters.
http://svn.netlabs.org/libc/browser/trunk/libc/src/libos2/safe
The thing we have to watch out for are the 1 or 2 api's that aren't wrapped like WinUpper.  Not every instance of the unsafe API's has to be wrapped.  There were some of the API's would always be called in a safe manner so I did not use the os2safe.h in those files.

Comment 39

[Peter Weilbacher]

Comment on attachment 244041 [details] [diff] [review]
same as the last but without silly syntax errors

I am kind of surprised that the easy workaround in that safe implementation actually has any effect. But if Knut says so... :-)


>Index: directory/c-sdk/configure.in
[...]
>-        DSO_LDOPTS='-Zomf -Zdll'
>+        DSO_LDOPTS='-Zomf -Zdll -Zhigh-mem'
>         _OPTIMIZE_FLAGS=-O3
>         _DEBUG_FLAGS="-g -fno-inline"
>         if test -n "$MOZ_OPTIMIZE"; then
>-          DSO_LDOPTS="$DSO_LDOPTS -Zlinker /EXEPACK:2 -Zlinker /PACKCODE -Zlinker /PACKDATA"
>+          DSO_LDOPTS="$DSO_LDOPTS -Zlinker /EXEPACK:2 -Zlinker /PACKCODE -Zlinker /PACKDATA -Zhigh-mem"

$DSO_LDOPTS from before is explicitely referenced, so you can remove that -Zhigh-mem here.

>Index: directory/c-sdk/config/OS2.mk
[...]
>-DLLFLAGS		= -g #-s
>-EXEFLAGS		= -g $(OMF_FLAG) -Zmtd -L$(DIST)/lib -o $@   # -s
>+DLLFLAGS		= -Zhigh-mem -g #-s
>+EXEFLAGS		= -Zhigh-mem -g $(OMF_FLAG) -Zmtd -L$(DIST)/lib -o $@   # -s
> ifeq ($(MOZ_OS2_EMX_OBJECTFORMAT),OMF)
>-EXEFLAGS                += -Zlinker /DE
>+EXEFLAGS                += -Zhigh-mem -Zlinker /DE

Same here (note the "+=").

>Index: gc/boehm/os_dep.c
[...]
>-    if (DosAllocMem(&result, bytes, PAG_EXECUTE | PAG_READ |
>-    				    PAG_WRITE | PAG_COMMIT)
>-		    != NO_ERROR) {
>+    APIRET rc = DosAllocMem(&result, bytes, PAG_EXECUTE | PAG_READ |
>+    				    PAG_WRITE | PAG_COMMIT | OBJ_ANY);
>+     if (rc)  /* If rc is not 0 then assume kernel did not handle OBj_ANY */
>+        {
>+           /* Try again without OBJ_ANY and if the first failure was not caused by
>+            * OBJ_ANY then we will get the same failure, else we have taken care of
>+            * pre-FP13 systems where the kernel couldn't handle it.
>+            */
>+           rc = DosAllocMem(&result, bytes, PAG_EXECUTE | PAG_READ |
>+    				    PAG_WRITE | PAG_COMMIT);
>+        }
>+    if rc != NO_ERROR) {

Thanks for the verbose comment, but as Mike phrased more nicely, the indentation with tabs and bracketing really sucks. ;-) It's a general rule in Mozilla code to use the same style as surrounding code (in this case open curly brackets at the end of a line) or what is listed in the emacs or vim lines at the very top (nothing here) and no tabs (although this file is not a good example of that). The same applies for all other C-code changes.

>Index: mailnews/base/src/nsMessengerOS2Integration.cpp


>Index: nsprpub/configure.in
[...]
>+        DSO_LDOPTS='-Zomf -Zdll -Zmap -Zhigh-mem'
>+        LDFLAGS='-Zmap -Zhigh-mem'
>         _OPTIMIZE_FLAGS="-O2 -s"
>         _DEBUG_FLAGS="-g -fno-inline"
>         if test -n "$MOZ_OPTIMIZE"; then
>-          DSO_LDOPTS="$DSO_LDOPTS -Zlinker /EXEPACK:2 -Zlinker /PACKCODE -Zlinker /PACKDATA"
>+          DSO_LDOPTS="$DSO_LDOPTS -Zlinker /EXEPACK:2 -Zlinker /PACKCODE -Zlinker /PACKDATA -Zhigh-mem"

See above.

>Index: nsprpub/pr/src/md/os2/os2io.c
[...]
>+    /*
>+     * All the pointer arguments (&file, &actionTaken and name) has to be in low memory for
>+     * DosOpen to use them.  Debugger showed that name was in high memory with -Zhigh-mem
>+     * The following moves name to low memory.
>+     */ 
>+    if ((ULONG)name >= 0x20000000)
>+    {
>+        size_t len = strlen(name) + 1;
>+        char *copy = (char *)alloca(len);
>+        memcpy(copy, name, len);
>+        name = copy;
>+    }
>+

Great, thanks for the comment. :-)

>Index: security/coreconf/OS2.mk
[...]
> ifdef BUILD_OPT
> OPTIMIZER		= -O2 -s
> DEFINES 		+= -UDEBUG -U_DEBUG -DNDEBUG
>-DLLFLAGS		= -DLL -OUT:$@ -MAP:$(@:.dll=.map)
>-EXEFLAGS    		= -PMTYPE:VIO -OUT:$@ -MAP:$(@:.exe=.map) -nologo -NOE
>+DLLFLAGS		= -DLL -OUT:$@ -MAP:$(@:.dll=.map) -Zhigh-mem
>+EXEFLAGS    		= -PMTYPE:VIO -OUT:$@ -MAP:$(@:.exe=.map) -nologo -NOE -Zhigh-mem
> OBJDIR_TAG 		= _OPT
>+LDFLAGS			+= -Zhigh-mem

If I compare this...

> #OPTIMIZER		= -O+ -Oi
> DEFINES 		+= -DDEBUG -D_DEBUG -DDEBUGPRINTS     #HCT Need += to avoid overidding manifest.mn 
>-DLLFLAGS		= -DEBUG -DLL -OUT:$@ -MAP:$(@:.dll=.map)
>-EXEFLAGS    		= -DEBUG -PMTYPE:VIO -OUT:$@ -MAP:$(@:.exe=.map) -nologo -NOE
>+DLLFLAGS		= -DEBUG -DLL -OUT:$@ -MAP:$(@:.dll=.map) -Zhigh-mem
>+EXEFLAGS    		= -DEBUG -PMTYPE:VIO -OUT:$@ -MAP:$(@:.exe=.map) -nologo -NOE -Zhigh-mem
> OBJDIR_TAG 		= _DBG
>-LDFLAGS 		= -DEBUG 
>+LDFLAGS 		= -DEBUG -Zhigh-mem 

...with this, should it not also be "=" above instead of "+="?

>Index: xpcom/io/nsLocalFileOS2.cpp
[...]
>     // upper-case the extension, then see if it claims to be an executable
>+#if 0 /* crashes with high memory */
>     WinUpper(0, 0, 0, ext);
>+#else
>+    strupr(ext);
>+#endif

Now that Mike has asked, too, perhaps add another line of comment about umlauts?



>Index: nsprpub/pr/src/md/os2/os2misc.c
[...]

>+/* os2safe.h has to be before os2.h, needed for high mem */
>+#include <os2safe.h>

Hmm, why is that in this file? I don't see os2.h included anywhere.

>Index: xpinstall/wizard/os2/uninstall/extra.c
[...]
>+/* os2safe.h has to be before os2.h, needed for high mem */
>+#include <os2safe.h>

Dito. (Perhaps we should not load the installer high?)

>Index: db/sqlite3/src/os_os2.c
[...]
>+/* os2safe.h has to be before os2.h, needed for high mem */
>+#include <os2safe.h>

If we need this in that file we have to get it into SQLite itself, too. And wrap it into an #if __GCC__ so that SQLite still compiles with OpenWatcom that doesn't have os2safe.h.

Whew! That's quite a patch to review. We are getting there, good work!

Perhaps we should also have Wan-Teh and/or Julien give the final version another look for the NSPR and NSS changes?

Comment 40

[timeless]

just for kicks, can the os/2 flavor of alloca fail? :)

Comment 41

[knut st. osmundsen]

no it can't ;-) 

alloca won't return NULL. The behavior should be very similar to that of Visual C++ and somewhat similar to GCC on linux.

Meaning, the allocation is read & write probed (page by page) before updating esp to make trigger any guard page exceptions expected by the CRT or OS. If the alloca request exceeds the available stack space, the result depends on what's next to it. If there's read+write accessible memory there, it'll be corrupted by the caller. If there isn't any committed memory there, or if it's no writable, then an exception will be raised and turn into a SIGSEGV should it reach LIBC.

Since the current linker organizes the stack object for the main thread next to the code object, the main thread will always fault in alloca and not corrupt anything. For secondary threads the behavior is undetermined in the current LIBC version. (I've planned to insert one or more non-present pages before (lower address) stack when implementing pthreads.)

Comment 42

[Peter Weilbacher]

(In reply to comment #38)
> The safe calls wrap the unsafe calls so that they are passed safe parameters.
> http://svn.netlabs.org/libc/browser/trunk/libc/src/libos2/safe

I saw that the header of os2safe.h says to link with -los2safe. Do we need to add that to the command line, too, or is that somehow implicit in -Zhigh-mem? (Would be great if these things were documented...)

Comment 43

[Peter Weilbacher]

(In reply to comment #33)
> After reading that DosStartSession wasn't safe for highmem in the newsgroup I
> asked Knut and he suggested using <os2safe.h>.  This wraps some calls that are
> unsafe.  I used it for all the DosStartSession calls.

Funny, DosStartSession is obviously implemented in libc as SafeDosStartSession (guessing by the SVN link to the source files) but not redefined in <os2safe.h>, so that workaround won't help much. A line
   #define DosStartSession SafeDosStartSession
should do it instead.

Comment 44

[Andy Willis (abwillis)]

No we don't need to link against that (which is a good thing as it doesn't exist ;) ).  They are in libos2 which I think is a default as I have never had to explicitly add it as I recall.
By the way, my os2safe.h does have:
#define DosStartSession         SafeDosStartSession
It is the last one there.
As to the files that don't have os2.h explicitly in them, they must be getting it from another header file that does call it so it is potentially needed in those files.  We could test which ones need it but I don't see the benefit.  os2misc.c as I recall had another possible problem besides dosstartsession.  I had done a search for all the redefined api's and os2misc.c I think it was had another one that could potentially be an issue.  The files I didn't add os2safe.h to that had some of these api calls were called in such a way as to not be a problem, if it was going to be or it was unclear I went ahead and added it.  I don't think it is worth the time to test if it is needed as safe is better than sorry IMO.  I thought of just adding it to the other files as there were only a hand full of other files that made calls to any of these api's.
As to:
 >+LDFLAGS			+= -Zhigh-mem
in:
security/coreconf/OS2.mk
I added the += here despite the + not being in the section below that because I added the LDFLAGS line and didn't want to remove anything that was potentially there which the debug probably does want to do.  
I am working on removing the extra spaces (mind you there were no tabs in my code).  I had tried to match the existing code structure and I think the new patch will be closer but I thought the first one matched so I am not 100% sure.

Comment 45

[Andy Willis (abwillis)]

Attachment: Fixed spacing issues and removed some extra -Zhigh-mems

Ok, I would rather error on the side of having an os2safe.h where it isn't strictly necessary than not have one that causes a problem by not having.  The installer file falls into that category.  As people have already used the installer successfully I would say that it is not necessary to have it there but I would rather error on the side of caution there.
Hopefully the spacing is liked better, I would guess it is as it isn't nearly as readable or easy to follow now.

Comment 46

[Peter Weilbacher]

(In reply to comment #44)
> By the way, my os2safe.h does have:
> #define DosStartSession         SafeDosStartSession
> It is the last one there.

Right, I had looked in the GCC322 tree instead. So this patch means that we would give up on 3.2.2 compatibility, otherwise we would need extra #ifs in the code to work around that and keep 3.2.2 builds low-mem only. Or perhaps a configure test that warns if somebody tries 3.2.2 instead of 3.3.5. Not sure Dave will be happy about that.

Regarding the spacing in the newest patch, I still see some problems but I could rectify that when checking it in. (My guess is that your editor tries to convert spaces to tabs or vice versa in some evil way.)


Additionally, I noticed that the OpenOffice 2 beta doesn't start when I am running my high-mem SeaMonkey while it works perfectly with a standard build. Can anyone confirm that observation? How do we figure out if that is our problem or theirs?

Comment 47

[Andy Willis (abwillis)]

I use ae.exe (now that I have a working copy of NEPMD I may switch to that), generally I don't see any changes in spaces and tabs.
I have no problem launching OO 2 beta here while Seamonkey with highmem is running.  

Comment 48

[Andy Willis (abwillis)]

The OBJ_ANY stuff should be safe with either compiler.  I could follow the __declspec test for the -Zhigh-mem as any that support __declspec have the -Zhigh-mem safe files.

Comment 49

[Andy Willis (abwillis)]

Rereading what you wrote about what you see in GCC 3.2.2 then I think we just need to add the define to os2safe.h.  We just add it to the build page that this needs to be added.

Comment 50

[Andy Willis (abwillis)]

I put the below in the newsgroup but wanted to add an update here so am adding it here.  As the below state, I found that there is a syntax error in the boehm code (missing opening parenthesis).  I am going to hold off on uploading a corrected patch until I see if there is anything else to address.
Below is from the newsgroup post.
Hmm, this got me to looking closer at my objdir and I realize that gc is not normally built although the directory structure and makefiles are.  I went in and ran make in boehm and find that I have a syntax error, I missed an opening parenthesis.
It just built but I have not idea now
 how to test it.  I don't know if we have ever used that or not.
A little searching pulls up:
http://www.mozilla.org/performance/leak-brownbag.html
It says, "Update (2006-06-13): The code integrating the Boehm GC in Mozilla has not been tested for a number of years and is unlikely to work anymore."
So I think for now just drop it from your tree, if we eventually put the code into the tree then I think we should probably update boehm as well.  If Boehm is never used again then no big deal, if it begins to be used again we would probably want it updated to work with the rest of the high-mem code. 

Comment 51

[Walter Meinl]

Andy, while Seamonkey seems to compile fine according to the newsgroup, I have difficulties with toolkit-apps (FF, TB) using the latest patch
D:/usr/src/mozilla/toolkit/xre/nsNativeAppSupportOS2.cpp: In member function `
   virtual nsresult MessageWindow::SendRequest(const char*)':
D:/usr/src/mozilla/toolkit/xre/nsNativeAppSupportOS2.cpp:676: error: `rc'
   undeclared (first use this function)
D:/usr/src/mozilla/toolkit/xre/nsNativeAppSupportOS2.cpp:676: error: (Each
   undeclared identifier is reported only once for each function it appears
   in.)
make.exe[5]: *** [nsNativeAppSupportOS2.o] Error 1

Though the patch itself applies fine here, and I'm sure to have all files affected freshly checked out before application

Comment 52

[Felix Miata]

I'd been running the 1.8.x branch nightlies, currently labeled 1.1b, for about 11 months. I keep mailnews and _many_ tabs in CZ and browser open the whole time. For the past several weeks, likely more than two months, it's been impossible to keep SM open for an extended time. Usually it's been crashing with "not enough memory for that operation" sometime between 24 and 60 hours uptime, generally a little under 48 hours. My newest SM popuplog entry is 23 Aug.

I started using Peter's highmem enabled 1.1b shortly after he made it available. I just shut it down manually after 104 hours uptime, and recovered 349M of RAM in doing so.

Comment 53

[Andy Willis (abwillis)]

I thought I had copied the xpfe code directly into the toolkit code as it is the same but I missed the APIRET on rc.  Added it and now xulrunner built fine here.  I'll wait another day or so to see if there is anything else that needs updating then put up a new patch with any updates that are needed.

Comment 54

[Andy Willis (abwillis)]

After doing a grep in the lib directory of gcc 3.2.2 I don't see that SafeDosStartSession was implemented at all.  I asked Knut and he didn't recall but said he may have added that later.  So adding the define to os2safe.h will not be enough.  We should be able to tie into the __declspec check in order to support the deprecated 3.2.2 compiler.  That will take me a bit more work to figure out.

Comment 55

[Andy Willis (abwillis)]

Another thought, Peter, I think we would be better to put in your config option.  Then if using 3.2.2 you have to use the option.  The advantage being that if we see something that may/may not be high-mem related we can turn it off easily and do a build without it for a test without having to hunt everything down.

Comment 56

[Peter Weilbacher]

(In reply to comment #54)
> After doing a grep in the lib directory of gcc 3.2.2 I don't see that
> SafeDosStartSession was implemented at all.

Yes, I think that was just not part of 3.2.2, the online SVN repository shows that SafeDosStartSession.c is one year newer than the others...

(In reply to comment #55)
> Another thought, Peter, I think we would be better to put in your config
> option.  Then if using 3.2.2 you have to use the option.  The advantage being
> that if we see something that may/may not be high-mem related we can turn it
> off easily and do a build without it for a test without having to hunt
> everything down.

If you want I can work and that and integrate my build-option change with your newest patch for trunk. Shouldn't be too hard.

Comment 57

[Andy Willis (abwillis)]

Sounds good to me... Boehm\os_dep.c needs the parenthesis and toolkit\xre\nsNativeAppSupportOS2.cpp needs the APIRET on the first rc.
Andy

Comment 58

[Peter Weilbacher]

Attachment: improve Andy's patch to include a new high-mem configure option

Hmm, this is somewhat shorter, probably because Andy's last patch also included some unrelated whitespace changes. Hope I didn't forget anything. (This is the patch against the current trunk, but I only tested building of a similar patch on 1.8 branch so far.)
The .mozconfig option to switch on high-memory support with this patch is "--enable-os2-high-mem".

Comment 59

[Dave Yeo]

Peter your latest patch (https://bugzilla.mozilla.org/attachment.cgi?id=244941) fails in directory/c-sdk.
patching file `directory/c-sdk/config/OS2.mk'
patching file `directory/c-sdk/config/autoconf.mk.in'
Hunk #1 succeeded at 128 (offset -4 lines).
patching file `directory/c-sdk/configure.in'
Hunk #1 FAILED at 954.
Hunk #2 succeeded at 1750 (offset -65 lines).
1 out of 2 hunks FAILED -- saving rejects to directory/c-sdk/configure.in#
patching file `security/manager/Makefile.in'

I just pulled these from trunk and I also saw the same failure the other day with Andy's patch. 
A quick look at c-sdk/configure.in shows you must have quite a different version.

Comment 60

[Peter Weilbacher]

There is a lot of stuff recently that I don't understand... For this patch I checked out fresh copies of the files involved from trunk and optimized the patch to include as few lines as possible. And retested applying the patch to those clean trunk files until it worked perfectly. Will look at it again.

Comment 61

[Dave Yeo]

Ok, my mistake. Browsing http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/directory/c-sdk/configure.in&rev=&root=/cvsroot
it seems for some reason I'm pulling the wrong configure.in and most likely others in mozilla/directory/c-sdk.
One other question, should there be another endif in this chunk from mozilla/security/manager/Makefile.in? I get an error about missing endif.
endif # WINNT
+ifeq ($(OS_ARCH),OS2)
+ifdef MOZ_OS2_HIGH_MEMORY
+DEFAULT_GMAKE_FLAGS += OS2_HIGH_MEMORY=1
+endif
 # OS_CFLAGS needs to be passed on down. 

Comment 62

[Andy Willis (abwillis)]

In Peter's patch c-sdk is against the c-sdk trunk... Mozilla is not using the c-sdk trunk but rather a branch version currently.  
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/directory/c-sdk/configure.in&rev=ldapcsdk_5_17_client_branch&mark=5.47.2.2
I failed in patching configure.in due to no rpath stuff that is seen in the patch.  Checking lxr I found no rpath (as seen in the above link) but there is in Dave's link.  Looking at the top of each, the one I went to shows, (ldapcsdk_5_17_client_branch:5.47.2.2).
I asked on irc in #developers and was told that Trunk mozilla does not use trunk c-sdk.  Peter's client.mk is different in this regard as that is what determines which c-sdk is pulled.

Comment 63

[Andy Willis (abwillis)]

Ok, I completed the build last night but just started using it this morning and see it is not using high-mem.  Will try to track it down today.

Comment 64

[Andy Willis (abwillis)]

There are two ways I have determined to be able to find if high-mem is in use.
1) The easiest way is the use of Theseus.  If high-mem is in use the Shared memory is the one that grows (Theseus sees the high-mem as shared mem).  Private memory grows a little with Seamonkey from 10M to 20M while shared mem grows towards a 100M or more depending on how long and what all is running).  Prior to using -Zhigh-mem the growth was reversed in private and shared.  Xulrunner the private memory is about 1M and all growth occurs in shared memory -  this makes me wonder if something is missing a -Zhigh-mem in Suite builds.

2) There is a logging libc (libc061.logchk).  If it is in use it creates a log file in the directory with the executable.  Search for __libc_HeapEndVoting if enmResult=4 the it's using high memory, if enmResult=2 then it's using lower memory for the default heap.  By default it logs everything and my log file was 300M so I used grep to find it, the following page says how to turn stuff off to make the log smaller.
http://svn.netlabs.org/libc/wiki/LibcLogging

Comment 65

[Peter Weilbacher]

Attachment: Fix version problems

(In reply to comment #62)
> Peter's client.mk is different in this regard as that is what
> determines which c-sdk is pulled.

The answer is that I simply used "cvs co" instead of "make -f client.mk pull" because I didn't want to go through pulling the whole source tree just to create a patch for 10 files. But I forgot to add the tags, sorry about that!

This addresses the version problems and adds the missing endif line to security/manager/Makefile.in.

I should also have noted before that I am a bit unhappy about the patch to sqlite's os_os2.c. In principle we would need another #ifdef MOZ_OS2_HIGH_MEMORY in there to make it consistent with the changes everywhere else, but as SQLite doesn't know about MOZ variables, that makes little sense. So I though that as os2safe.h probabyl does not do any harm in itself.

Comment 66

[Andy Willis (abwillis)]

I think I found what is missing as to why it is not building high.
In the configure.in under nsprpub and directory\c-sdk there is no 
AC_DEFINE(MOZ_OS2_HIGH_MEMORY) so that 
MOZ_OS2_HIGH_MEMORY = @MOZ_OS2_HIGH_MEMORY@
is what shows in the autoconf under nsprpub.
I can't build to test for certain as I had done a cvs pull and get an error as Peter updated in:
https://bugzilla.mozilla.org/show_bug.cgi?id=345517 
But I just added the above into the ifdef structure.

Comment 67

[Peter Weilbacher]

(In reply to comment #66)
> I think I found what is missing as to why it is not building high.
> In the configure.in under nsprpub and directory\c-sdk there is no 
> AC_DEFINE(MOZ_OS2_HIGH_MEMORY) so that 
> MOZ_OS2_HIGH_MEMORY = @MOZ_OS2_HIGH_MEMORY@
> is what shows in the autoconf under nsprpub.

I don't understand. That replacement woudl be done by AC_SUBST and that is there, without condition, so it should always get replaced -- unless the patch didn't apply correctly... AC_DEFINE is only called when high memory was actually requested. Did you specify "ac_add_options --enable-os2-high-mem" in your .mozconfig?

Comment 68

[Andy Willis (abwillis)]

Yes, I did indeed add "ac_add_options --enable-os2-high-mem" to my .mozconfig.
The autoconf.mk in objdir\config shows as correct (MOZ_OS2_HIGH_MEMORY = 1).  The autoconf.mk files in objdir\nsprpub\config and directory\c-sdk\config showed MOZ_OS2_HIGH_MEMORY = @MOZ_OS2_HIGH_MEMORY@.
My build actually did get far enough to show it did correct this issue.

dnl ========================================================
dnl Enable high-memory support on OS/2, disabled by default
dnl ========================================================
AC_ARG_ENABLE(os2-high-mem,
    [  --enable-os2-high-mem Enable high-memory support on OS/2],
    [ if test "$enableval" = "yes"; then
        MOZ_OS2_HIGH_MEMORY=1
        AC_DEFINE(MOZ_OS2_HIGH_MEMORY)
      fi ])
AC_SUBST(MOZ_OS2_HIGH_MEMORY)

Adding         AC_DEFINE(MOZ_OS2_HIGH_MEMORY) above to each of the other configures corrects the issue.

Comment 69

[Peter Weilbacher]

*** Bug 358636 has been marked as a duplicate of this bug. ***

Comment 70

[Peter Weilbacher]

OK, for the xpicleanup and the GnuPG problem a similar workaround as in os2io.c works:

+#ifdef MOZ_OS2_HIGH_MEMORY
+    if ((ULONG)path >= 0x20000000)
+    {
+        size_t len = strlen(path) + 1;
+        char *copy = (char *)alloca(len);
+        memcpy(copy, path, len);
+        path = copy;
+    }
+#endif
     rc = DosQueryAppType(path, &ulAppType);

I also had the idea for the os_os2.c addition for SQLite: there we can just make up another define (like OS2_HIGH_MEMORY) and pass it to the compiler through the SQLite Makefile. So, in a sense this would work similar to what we do in this patch for NSPR, Directory, and NSS.

Will upload a new patch once I have looked more closely at why Andy's problem from comment 68.

Comment 71

[Andy Willis (abwillis)]

After finally finding a fix for the build break I was just just about to look at applying the fix from os2io.c but I see you beat me to that.  I think the reason that the comment #68 issue is true (though I really don't understand configure that well) is that after it is defined it then has to be substituted (AC_SUBST) for it to end up correctly in the mk file otherwise it is taken literally.

Comment 72

[Peter Weilbacher]

Attachment: complete patch for high-mem v4

OK, this solves the problem with DosQueryAppType(), improves the variable setting in the configure.in files (I checked by building trunk with and without high-mem switch this time), and adds proper detection to SQLite's os_os2.c (that I will also get upstream like this).

This patch still contains the #warning lines, that I would remove before checking the patch in.

Comment 73

[Peter Weilbacher]

Wan-Teh, Julien, it would be nice if you could take a look at the latest patch from the point of view of NSPR and NSS. Does it make sense like this at all?

(Who do I have to ask for the stuff in LDAP?)

Comment 74

[Andy Willis (abwillis)]

Now that I can get past the build break I find that in xre\toolkit\nsnativeappsupportos2.cpp there is a missing }
#ifdef MOZ_OS2_HIGH_MEMORY
        APIRET rc = DosAllocSharedMem( &pvData, NULL, ulSize,
                                       PAG_COMMIT | PAG_READ | PAG_WRITE | OBJ_GETTABLE | OBJ_ANY);
        if (rc != NO_ERROR) { // Did the kernel handle OBJ_ANY?
            // Try again without OBJ_ANY and if the first failure was not caused by
            // OBJ_ANY then we will get the same failure, else we have taken care of
            // pre-FP13 systems where the kernel couldn't handle it.
            rc = DosAllocSharedMem( &pvData, NULL, ulSize,
                                    PAG_COMMIT | PAG_READ | PAG_WRITE | OBJ_GETTABLE);
            if (rc != NO_ERROR) {
                return NS_ERROR_OUT_OF_MEMORY;
            }
        } <==========================================  missing.
Have just found it so haven't got further but I think that everything else should be common to Seamonkey which built fine.

Comment 75

[Peter Weilbacher]

Args, I have never done a patch before that needed to be tested in so many configurations, so thanks for the hint Andy. I would however like to get some more fundamental feedback (especially on the NSPR, NSS, LDAP changes) before uploading yet another version.

Comment 76

[Andy Willis (abwillis)]

I understand wanting to wait to see if there is more before uploading a new patch :)  Can't answer from NSPR, NSS, LDAP perspective but the brace is all that I found wrong from a build perspective.  I had already built Seamonkey and now I have built xulrunner and both are using high-mem now.  I had been seeing some issues where Chatzilla running on xulrunner would not launch links into Seamonkey when clicked on... I don't know if it was high-mem related but if so then it must have been the DosQueryAppType stuff as it is now working fine.  I had also not been able to use the little icon to the left to drag url's to the desktop and that is now working so either unrelated bugs were fixed or high-mem stuff is coming along.  I still have yet to build without the high-mem but expect those to work fine, but will try to build today.

Comment 77

[Wan-Teh Chang]

Comment on attachment 246566 [details] [diff] [review]
complete patch for high-mem v4

Peter, you can use the Mozilla Module Onwers page
(http://www.mozilla.org/owners.html) to find the right
people to review a patch.  The owners of LDAP are listed
at http://www.mozilla.org/owners.html#directory-sdk

Your changes to NSPR and NSS are fine.  There are a few
minor improvements you can make.
1. You can remove the debug output statements such as
'echo' and '#warning' when this has been fully debugged.
2. Some variables are defined but not used, for example,
the make variable MOZ_OS2_HIGH_MEM in NSPR's autoconf.mk.in.
3. In NSPR's configure.in, you can rewrite this if-else
statement

>+        if test -n "$MOZ_OS2_HIGH_MEMORY"; then
>+          echo "Using high memory"
>+          DSO_LDOPTS='-Zmap -Zomf -Zdll -Zhigh-mem'
>+          LDFLAGS='-Zmap -Zhigh-mem'
>+          AC_DEFINE(MOZ_OS2_HIGH_MEMORY)
>+        else
>+          echo "NOT using high memory"
>+          DSO_LDOPTS='-Zmap -Zomf -Zdll'
>+          LDFLAGS='-Zmap'
>+        fi

as follows:

          DSO_LDOPTS='-Zomf -Zdll -Zmap'
          LDFLAGS='-Zmap'
          if test -n "$MOZ_OS2_HIGH_MEMORY"; then
            echo "Using high memory"
            DSO_LDOPTS="$DSO_LDOPTS -Zhigh-mem"
            LDFLAGS="$LDFLAGS -Zhigh-mem"
            AC_DEFINE(MOZ_OS2_HIGH_MEMORY)
          fi

There are other similar changes you can make.
4. In security/coreconf/OS2.mk, you can define a new variable,
say HIGHMEM_LDFLAG, whose value is empty by default and -Zhigh-mem
if $(OS2_HIGH_MEMORY) is 1.  Then simply use $(HIGHMEM_LDFLAG) in
DSO_LDOPTS, DLLFLAGS, EXEFLAGS, and LDFLAGS.

Comment 78

[Mike Kaply [:mkaply]]

Peter, don't feel that this patch needs to go in all at once.

Why don't we go ahead and get the #ifdefs in the code in for the high mem stuff and we can work on getting the other stuff reviewed.

What did you think of wtc's comments?

Comment 79

[Peter Weilbacher]

Yes, wtc's comments -- as always -- were very helpful. I just didn't find time so far to create another patch. Your suggestion to split it up into smaller pieces is a good one and will help with that, too.

Comment 80

[Peter Weilbacher]

Attachment: Changes to OS/2 files [Checked into trunk]

OK, let's start with changes to the OS/2 files.

Changes from the earlier patches are:
- removed debug #warnings
- wrapped comments to 80 lines
- in nsDragService.cpp I realized that the ERROR_ALREADY_EXISTS
  condition would be handled in the following code

Comment 81

[Peter Weilbacher]

Attachment: NSPR changes [Checked into NSPR trunk and Mozilla trunk]

The NSPR changes. These now remove the build debugging stuff and hopefully follow all of wtc's recommendations.

Comment 82

[Peter Weilbacher]

Attachment: NSS changes

The NSS changes. These hopefully follow all of wtc's recommendations.

Comment 83

[Peter Weilbacher]

Attachment: Mozilla build changes [Checked into trunk]

Changes to the Mozilla build system. These were also adapted following earlier comments.

Comment 84

[Peter Weilbacher]

Attachment: Changes to SQLite build and OS/2 code

Changes to SQLite in the Mozilla tree. If this is OK, I will make the change to os_os2.c simultaneously to the Mozilla tree and the SQLite repository.

Comment 85

[Peter Weilbacher]

Attachment: Changes to directory/LDAP build config

Changes to directory. Also improved following wtc's comments.

Comment 86

[Andy Willis (abwillis)]

The ldap patch I think needs +AC_SUBST(MOZ_OS2_HIGH_MEMORY) added to it.  I am still in the process of updating my tree so haven't tested anything yet, just saw that as I was going through the patches.

Comment 87

[Andy Willis (abwillis)]

Please forgive the spam from the last update... I looked through whole patch more than once and still didn't see that it was down below.

Comment 88

[Andy Willis (abwillis)]

Looking at the directory/c-sdk/config/OS2.mk file I see a possible issue here:
ifdef BUILD_OPT
OPTIMIZER		= -O3
DLLFLAGS		= 
EXEFLAGS    		= -Zmtd -o $@
else
OPTIMIZER		= -g #-s
DLLFLAGS		= -g #-s
EXEFLAGS		= -g $(OMF_FLAG) -Zmtd -L$(DIST)/lib -o $@   # -s
ifeq ($(MOZ_OS2_EMX_OBJECTFORMAT),OMF)
EXEFLAGS                += -Zlinker /DE

The EXEFLAGS Line under #ifdef BUILD_OPTS has an = and not += so our -Zhigh-mem will be lost.  Can we just move our check to below this?

Comment 89

[Mike Kaply [:mkaply]]

Comment on attachment 248066 [details] [diff] [review]
Changes to OS/2 files
[Checked into trunk]

sr=mkaply (OS/2 only)

Comment 90

[Mike Kaply [:mkaply]]

Comment on attachment 248067 [details] [diff] [review]
NSPR changes [Checked into NSPR trunk and Mozilla trunk]

Aren't we leaking a string in all these cases?

Comment 91

[Andy Willis (abwillis)]

I don't know enough about autoconf to know but shouldn't we still have the autoconf.mk changes for nspr and nss and still do the AC_SUBST in configure.in?  Or is the AC_DEFINE enough?  

Comment 92

[Andy Willis (abwillis)]

Ok, I had tried checking earlier on IRC but had no responses... I just checked again (probably asked the question better this time) and it looks like AC_DEFINE should be enough as it updates the mozilla-config.h while AC_SUBST updates the autoconf.mk file and as there are no changes to makefiles that are dependent on these changes then the autoconf.mk file should not need to be updated.  

Mike, I checked with Knut about the strings:
<Bird>	abwillis: no, we aren't. alloca() allocates memory from the stack and it's "freed" automatically when we return from the function.

Comment 93

[Andy Willis (abwillis)]

Comment on attachment 248072 [details] [diff] [review]
Changes to SQLite build and OS/2 code

The sqlite patch looks good and built fine.. the only thing that looks wrong is the directory/c-sdk/config/OS2.mk
in comment #88

Comment 94

[Peter Weilbacher]

Comment on attachment 248066 [details] [diff] [review]
Changes to OS/2 files
[Checked into trunk]

OK, this is in.

Comment 95

[Peter Weilbacher]

I think you answered all the questions and comment yourself while I was sleeping, so I only need to update the patch for directory. I think I should use the NSS approach that Wan-Teh suggested of defining a new variable.

I also contacted Daniel Kruse to check the SQLite patch with OpenWatcom, and while we both have some problems building the newest version, the patch doesn't hurt.

Comment 96

[Peter Weilbacher]

Attachment: Changes to directory/LDAP build config [Checked into ldapcsdk_5_17_client_branch and LDAP trunk)

OK, the high-mem stuff is now only in the GCC section and should not get overridden any more.

Comment 97

[Mike Kaply [:mkaply]]

Comment on attachment 248067 [details] [diff] [review]
NSPR changes [Checked into NSPR trunk and Mozilla trunk]

r=mkaply

Comment 98

[Mark Smith [:mcs]]

Comment on attachment 248158 [details] [diff] [review]
Changes to directory/LDAP build config [Checked into ldapcsdk_5_17_client_branch   and LDAP trunk)

I do not know anything about OS/2 high memory, but I don't think the directory patch will break anything.  I am not a superreviewer though.

Comment 99

[Peter Weilbacher]

Comment on attachment 248158 [details] [diff] [review]
Changes to directory/LDAP build config [Checked into ldapcsdk_5_17_client_branch   and LDAP trunk)

Thanks for looking at it Mark. But if you are not doing sr for directory who is? You are listed as owner at <http://www.mozilla.org/owners.html#directory-sdk>. Or is sr not necessary for directory? Then you could mark "addl. review+" if you are happy with the variable naming etc. :-)

I think I am allowed to check in this patch once mkaply is happy. But do I need to make another one for directory-trunk and check in there as well?

Comment 100

[Mark Smith [:mcs]]

(In reply to comment #99)
> (From update of attachment 248158 [details] [diff] [review] [edit])
> Thanks for looking at it Mark. But if you are not doing sr for directory who
> is?

I CC'd Rich Megginson on this bug since lately he has been leading the effort to manage the LDAP code.

In the past, people like Dan Mosedale have filled the super reviewer role.  A super review is needed if you are landing code that directly affects the Mozilla clients; typically, we have not required a super review for LDAP trunk code (since we use a branch of the LDAP code in clients such as SeaMonkey and TBird and only sync up once in a while).  In practice, as module owner for the LDAP code, I generally review every significant change as well.

> But do I need to make another one for directory-trunk and check in there
> as well?

Yes, I think you should also land this on the trunk (I don't think we plan to merge from the branch used by the Mozilla clients back into the LDAP trunk).

Comment 101

[Wan-Teh Chang]

Comment on attachment 248070 [details] [diff] [review]
Mozilla build changes [Checked into trunk]

r=wtc on the patch "Mozilla build changes".

Comment 102

[Wan-Teh Chang]

Comment on attachment 248067 [details] [diff] [review]
NSPR changes [Checked into NSPR trunk and Mozilla trunk]

r=wtc on the patch "NSPR changes".  Good patch.

This comment in os2io.c has a typo:

  All the pointer arguments (&file, &actionTaken and name)
  has to be in

Change "has" to "have".

Nit: I'd move the code that creates low-memory copies of 'name'
and 'path' to the beginning of the functions _PR_MD_OPEN
and _PR_CreateOS2Process to ensure that if new code is added
in the future, we still move 'name' and 'path' to low memory
before the first use of 'name' and 'path'.

Comment 103

[Wan-Teh Chang]

Comment on attachment 248068 [details] [diff] [review]
NSS changes

r=wtc on the patch "NSS changes".

Peter, why don't we also name the make variable MOZ_OS2_HIGH_MEMORY,
with the MOZ_ prefix, in NSS/coreconf?  This would be more consistent
with the other build systems in Mozilla.

Comment 104

[Peter Weilbacher]

Comment on attachment 248067 [details] [diff] [review]
NSPR changes [Checked into NSPR trunk and Mozilla trunk]

OK, I followed wtc's comments and moved the new code closer to the start of the functions, corrected the typo, and then checked it into NSPR trunk and Mozilla trunk (NSPRPUB_PRE_4_2_CLIENT_BRANCH).

Comment 105

[Peter Weilbacher]

Attachment: NSS changes, updated [Checked into NSS trunk, NSS_3_11_BRANCH]

(In reply to comment #103)
> Peter, why don't we also name the make variable MOZ_OS2_HIGH_MEMORY,
> with the MOZ_ prefix, in NSS/coreconf?  This would be more consistent
> with the other build systems in Mozilla.

Yes, originally I thought that I should drop the MOZ prefix for the "external" modules and somehow I failed to change that for NSS. (The only place where it does make sense is the SQLite code and there I left it without MOZ_ on purpose.)

This is the new patch, carrying over sr+.

For this I also need help with the checkin once mkaply agrees.

Comment 106

[Mike Kaply [:mkaply]]

Comment on attachment 248070 [details] [diff] [review]
Mozilla build changes [Checked into trunk]

r=mkaply

Comment 107

[Mike Kaply [:mkaply]]

Comment on attachment 248158 [details] [diff] [review]
Changes to directory/LDAP build config [Checked into ldapcsdk_5_17_client_branch   and LDAP trunk)

r=mkaply

Comment 108

[Peter Weilbacher]

Are the configure files in nsprpub and directory/c-sdk not automatically recreated from the changed configure.in files? The OS/2 version of autoconf 2.13 gives a very different result, so I cannot easily checkin the changes...

Comment 109

[Rich Megginson]

(In reply to comment #108)
> Are the configure files in nsprpub and directory/c-sdk not automatically
> recreated from the changed configure.in files? The OS/2 version of autoconf
> 2.13 gives a very different result, so I cannot easily checkin the changes...

I usually use autoconf-2.13 on Red Hat Enterprise Linux or Fedora Core to regenerate configure from configure.in, for the c-sdk anyway (I don't know about NSPR).  I've also used autoconf-2.13 on Windows cygwin.

How different is it?  In what way(s) is it different?

Comment 110

[Peter Weilbacher]

Different enough, containing non-standard stuff about path seperators, executable extensions and some other stuff that I don't understand.

As for statistics:
$ wc -l configure.orig configure.os2
   6306 configure.orig
   6540 configure.os2
$ diff -u configure.orig configure.os2 | grep "^+" | wc -l
    393
$ diff -u configure.orig configure.os2 | grep "^-" | wc -l
    159

But I could try to resurrect my Mozilla build environment on one the Linux machines at work, and see what results I get there.

Comment 111

[Rich Megginson]

(In reply to comment #110)
> Different enough, containing non-standard stuff about path seperators,
> executable extensions and some other stuff that I don't understand.
> 
> ...
> 
> But I could try to resurrect my Mozilla build environment on one the Linux
> machines at work, and see what results I get there.

I think that would be best.  I don't think I trust the OS/2 port of autoconf according to that output . . .

Comment 112

[Wan-Teh Chang]

Peter, NSPR's configure file is not automatically
recreated from the changed configure.in file.  You can
use the OS/2 version of autoconf-2.13.  The autoconf 2.13
in Fedora or Red Hat Enterprise Linux has additional patches
applied, and therefore generates different output.  I
prefer the standard autoconf-2.13 (explained below), but
either one works.

Do you want me to take care of this for you?  There is a
post-processing step, to filter out '\015', for MKS Toolkit
support.  I know how to do that.

The reason I prefer the standard autoconf-2.13 is that
its output can be easily made to support MKS Toolkit (a
commercial product that provides Unix command-line tools
for Windows).

Comment 113

[Peter Weilbacher]

Thanks for the suggestions.

I found that we don't have a Linux machine with such an old autoconf installed by default, so I compiled autoconf 2.13 in my home directory. When run against directory/c-sdk/configure.in the resulting configure file is missing some two blocks starting with "for ac_declaration in" compared with the in-tree version.

For nsprpub/configure.in I don't see any differences in the file (apart from the new OS/2 bits and line-no. changes) but no \015 chars appear, either. So, yes, Wan-Teh, if you could do the conversion and check in the changes, that would be great.

Comment 114

[Wan-Teh Chang]

Peter, I checked in NSPR's configure file on the NSPR trunk and the
Mozilla trunk (NSPRPUB_PRE_4_2_CLIENT_BRANCH).

Comment 115

[Wan-Teh Chang]

Comment on attachment 248072 [details] [diff] [review]
Changes to SQLite build and OS/2 code


>+#if __GNUC__ >= 3 && __GNUC_MINOR__ >= 3 && defined(OS2_HIGH_MEMORY)

If you meant to say "if GCC version >= 3.3" here,
this is incorrect.  You need to say:

#if (__GNUC__ > 3 || __GNUC__ == 3 && __GNUC_MINOR__ >= 3) && defined(OS2_HIGH_MEMORY)

Comment 116

[Peter Weilbacher]

Attachment: Changes to SQLite build and OS/2 code, updated [Checked into trunk]

(In reply to comment #115)
> If you meant to say "if GCC version >= 3.3" here,
> this is incorrect.  You need to say:
> 
> #if (__GNUC__ > 3 || __GNUC__ == 3 && __GNUC_MINOR__ >= 3) && 
> defined(OS2_HIGH_MEMORY)

Ah, to include GCC 4.0 to 4.2, indeed. It may be quite a while until we get one of those under OS/2 so I didn't think about that, but it makes sense. Thanks for catching that!

This new patch follows your recommendation. Carrying over Andy's r+.

Comment 117

[Peter Weilbacher]

(In reply to comment #113)
> I found that we don't have a Linux machine with such an old autoconf installed
> by default, so I compiled autoconf 2.13 in my home directory. When run against
> directory/c-sdk/configure.in the resulting configure file is missing some two
> blocks starting with "for ac_declaration in" compared with the in-tree version.

Well, I checked that one in to both LDAP trunk and ldapcsdk_5_17_client_branch and all tinderboxes seem to have survived it, so the plain autoconf-2.13 seems to work, too, even if it is missing those blocks...

Comment 118

[Andy Willis (abwillis)]

I just updated to the new mzfntcfg (static linked) and glibidl (single directory) and now my builds are no longer using high-mem.  I checked everything and even added a few extra -Zhigh-mem's to various places and they had no effect.  I checked the autoconf.mk files and they all show that -Zhigh-mem is being added correctly.  Will have to look into it more tomorrow.  If I can't figure it out I will try going back to the older mzfntcfg (don't think glibidl would affect it) and see if that changes it back.  

Comment 119

[Peter Weilbacher]

So, how do you determine if a build is using high-mem or not in practise?

Comment 120

[Andy Willis (abwillis)]

The easiest way I have found is to use Theseus.  Theseus apparently doesn't understand the high-mem fully so if high-mem is being used it shows the memory as shared instead of private.  In builds that are not using high-mem when you first launch Mozilla there will be ~50M in Private memory and `5M in shared (the amount in shared will be ~10 in a dynamic build).  When more memory is used the Private memory will increase (shared memory usually goes up to 10 and 15 respectively after some time).  With a high-mem build those numbers are reversed and Shared memory shows as ~50 to start and it grows with very little growth in Private memory.  I confirmed this by the second method in comment #64 which shows absolutely whether it is in use or not.

Comment 121

[Peter Weilbacher]

Comment on attachment 248681 [details] [diff] [review]
Changes to SQLite build and OS/2 code, updated [Checked into trunk]

OK, checked the SQLite patch into trunk.

By now I also checked the os_os2.c change into SQLite's CVS upstream.

Comment 122

[Peter Weilbacher]

(In reply to comment #120)
> The easiest way I have found is to use Theseus.
[...]
> With a high-mem build those numbers are
> reversed and Shared memory shows as ~50 to start and it grows with very little
> growth in Private memory.

Thanks. I read a few days ago that one can also look at the Process -> Private/Shared Objects Summary in Theseus. In highmem builds the private stuff by libc061 gets loaded above address 2000000.

> I confirmed this by the second method in comment #64
> which shows absolutely whether it is in use or not.

Right, libc061.logchk helps, too. In the log it creates one can also see that std_malloc and other functions allocate memory above 0x2000000 in highmem builds.

Comment 123

[Dave Yeo]

In reply to comment #122
Peter is that 2000000 or 20000000? I take it this is hex in which case 0x20000000 is close to 512 MB which makes more sense.
Using a recent trunk built with --enable-os2-high-mem I see very little shared objects above 20000000. Just Innowin, Mailnews, libc06b5, ft2lib, libc061, and device driver #1. Private is even less, Innowin, libc06b5 and libc061.
Another perhaps related observation is that I removed the Innotek font engine and Seamonkey became very unstable. Reinstalling the font engine brought back stability.

Comment 124

[Mike Kaply [:mkaply]]

Comment on attachment 248442 [details] [diff] [review]
NSS changes, updated [Checked into NSS trunk, NSS_3_11_BRANCH]

r=mkaply

Comment 125

[Peter Weilbacher]

(In reply to comment #123)
> Peter is that 2000000 or 20000000? I take it this is hex in which case
> 0x20000000 is close to 512 MB which makes more sense.

Yes, sure, with seven zeros...

> Using a recent trunk built with --enable-os2-high-mem I see very little shared
> objects above 20000000. Just Innowin, Mailnews, libc06b5, ft2lib, libc061, and
> device driver #1. Private is even less, Innowin, libc06b5 and libc061.

I am not very familiar with how the library loader works, but I don't think that it would load the DLLs themselves high. Most of the big bits of the SEAMONKEY process that I see with Theseus are loaded high, both for private and shared memory, mostly listing LIBC061.DLL as the responsible module. So I think the patches here do what they should.

> Another perhaps related observation is that I removed the Innotek font engine
> and Seamonkey became very unstable. Reinstalling the font engine brought back
> stability.

And that is only true for a highmem build? Hmm, perhaps we missed some bad API in the GFX code that is in the code path for non-FT2LIB operation. But I think we should address any additional problems in follow-up bugs. If you have a specific scenario for crashes without FT2LIB, it would be great if you could file a bug.

Comment 126

[Wan-Teh Chang]

Comment on attachment 248442 [details] [diff] [review]
NSS changes, updated [Checked into NSS trunk, NSS_3_11_BRANCH]

I checked in the security/coreconf/OS2.mk change
on the NSS trunk (NSS 3.12) and the
NSS_3_11_BRANCH (NSS 3.11.5).

I omitted the line
+DEFINES                 += -DMOZ_OS2_HIGH_MEMORY=1
because it's not being used.

Peter, you can check in the change to
security/manager/Makefile.in yourself.

Checking in OS2.mk;
/cvsroot/mozilla/security/coreconf/OS2.mk,v  <--  OS2.mk
new revision: 1.26; previous revision: 1.25
done

Checking in OS2.mk;
/cvsroot/mozilla/security/coreconf/OS2.mk,v  <--  OS2.mk
new revision: 1.24.2.2; previous revision: 1.24.2.1
done

Comment 127

[Wan-Teh Chang]

If it is fine to include <os2safe.h> in a non-highmem build,
it seems that the code will be simpler if we add the highmem
code *unconditionally*.  The highmem code should be harmless
in the non-highmem build.  Then we can get rid of -DMOZ_OS2_HIGH_MEMORY.

Comment 128

[Peter Weilbacher]

Yes, you are right that in principle it would be simpler without wrapping os2safe.h. But as there is some slowdown by wrapping several OS/2 APIs, we thought it would be safer for now, to clearly seperate _all_ changes we make for the highmem build. In a subsequent step, that might take a few months depending on the testing we get from the trunk, we can then decide to make high-memory the default and remove the old code paths. I can file a follow-up bug so that it doesn't get forgotten.

Comment 129

[Peter Weilbacher]

Comment on attachment 248442 [details] [diff] [review]
NSS changes, updated [Checked into NSS trunk, NSS_3_11_BRANCH]

OK, checked the security/manager/Makefile.in changes into trunk. (If I see correctly, there is not branch that this needs to get into.)

Comment 130

[Peter Weilbacher]

OK, with that all parts are checked in, and this bug is fixed. If there are problems, let's address them in new bugs.

Thanks everybody for testing, help, reviews, checkins, and especially Andy for getting the whole thing started and doing the first patches.

Comment 131

[Andy Willis (abwillis)]

I just noticed that gc/boehm/os_dep.c did not update properly in CVS.  It shows in cvs log that it should be but when I did a cvs -diff it didn't show up correctly so I checked lxr and it doesn't show up correctly there even though cvs log says it should be.  Of course I don't know if boehm is actually likely to be used on OS/2.  The cvs diff showed lots of whitespace changes in that file now, I tried to rename it and do a pull but haven't gotten it to pull so far.

Comment 132

[Wan-Teh Chang]

(In reply to comment #129)

Peter, yes, security/manager/Makefile.in is outside NSS, so you don't
need to check its change into any NSS branch.

Comment 133

[Peter Weilbacher]

(In reply to comment #131)
> I just noticed that gc/boehm/os_dep.c did not update properly in CVS.  It shows
> in cvs log that it should be but when I did a cvs -diff it didn't show up
> correctly so I checked lxr and it doesn't show up correctly there even though
> cvs log says it should be.  Of course I don't know if boehm is actually likely
> to be used on OS/2.  The cvs diff showed lots of whitespace changes in that
> file now, I tried to rename it and do a pull but haven't gotten it to pull so
> far.

Perhaps you checked gc/boehm out from some branch by mistake, or you did a dated or versioned checkout? I just did

   cd \
   mkdir test
   cd test
   cvs -z3 co mozilla/gc/boehm/
   vi mozilla\gc\boehm\os_dep.c

and it shows up correctly, as do bonsai's CVS log and CVS diff
http://bonsai.mozilla.org/cvsview2.cgi?file=os_dep.c&branch=&subdir=/mozilla/gc/boehm&command=DIFF_FRAMESET&rev1=1.15&rev2=1.16

But yes, it's a bit confusing that LXR doesn't show the changes, but it seems to specifically index v1.12 instead of v1.16.