283316 - SQLITE not working on OS/2

Status: VERIFIED FIXED

(Core :: SQLite and Embedded Database Bindings, defect)

Description

[Andy Willis (abwillis)]

I built sunbird from the Trunk and it built fine but then none of the dialogs
work except create new calendar and it won't actually create one as the finish
button just highlights.  RMB brings up popups but none of the choices do
anything but hightlight.  I built a debug build and get these each time one of
the options are pressed:
JavaScript error: chrome://calendar/content/calendar.xul, line 1:
gCalendarWindow has no properties
Asked about this on #calendar and confirmed I am not the only person
experiencing this.

Comment 1

[Andy Willis (abwillis)]

Attachment: Log from debug session.

Comment 2

[Simon Paquet [:sipaq]]

Andy, can you test this with the new trunk nightly at
http://www.babylonsounds.com/sunbird/Sunbird_20050320_Nightly.zip

Comment 3

[Michiel van Leeuwen (email: mvl+moz@)]

The dialog has been turned into a wizard, and now works for me.

Comment 4

[Andy Willis (abwillis)]

Reopening and marking OS/2 only as the reference in comment #3 I am assuming is
windows.  I see the same thing now that I did when opening the bug (the dialogs
look the same and is what I think is called a wizard).  The same problem exists
in the calendar builds in SeaMonkey (I build it about once a week or so and it
has persisted).  My first attempt to build Sunbird since the bug was opened was
when this was closed and it is still there.  I realized that all my builds had
been static (including Sunbird) so I built Sunbird without the --enable-static
--disable-shared and that made no difference.  I am now building a
--enable-debug --disable-optimize to try to track it down.

Comment 5

[Andy Willis (abwillis)]

Attachment: sqlite os2 specific header

Comment 6

[Andy Willis (abwillis)]

Attachment: update os.h and makefile.in

Comment 7

[Andy Willis (abwillis)]

Attachment: os2 specific os file

There were no OS/2 specific files, created them from a patch to the 2.8 port.
With these I can now create a calendar but no rigorous testing done to sqlite.

Comment 8

[Andy Willis (abwillis)]

Changing summary from:
Sunbird dialogs are not working
to SQLITE not working as that seems to be the root cause.
Not sure that this could be considered a regression under the circumstances
other than that sunbird worked prior to using sqlite on OS/2.

Comment 9

[Mike Kaply [:mkaply]]

Attachment: Better os.h diff?

Comment 10

[Mike Kaply [:mkaply]]

Attachment: More context in os.h diff

Comment 11

[Andy Willis (abwillis)]

Attachment: update to 3.2.2 sqlite like the rest of the platforms

Comment 12

[Mike Kaply [:mkaply]]

Attachment: Just the new changes

Comment 13

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 200225 [details] [diff] [review]
Just the new changes

r=vladimir -- looks fine to me; as well as trying to get this upstream, it
would be helpful to attach a full diff of os2 changes from stock sqlite so that
I don't break os2 when we do a sqlite upgrade (one's sitting on my to-do list
at the moment).

Comment 14

[Mike Kaply [:mkaply]]

Last two fixes checked in.

Andy,

Can you reponsd to vlad's comment?

Thanks

Comment 15

[Andy Willis (abwillis)]

Attachment: Remove extra whitespace

The whitespace was more than I could take ;)

Comment 16

[Andy Willis (abwillis)]

Attachment: Here are all the changes that were done to get sqlite to work with OS/2

The os_os2.c and os_os2.h files were both new so I don't know that the patch is
the correct way to do this but what I did was create a blank file and did a
diff against it for those 2 files.

Comment 17

[Peter Weilbacher]

Attachment: build bustage fix

I just tried to build with --enable-calendar and found that os_os2.c didn't compile, it seems one function definition got out of sync. This patch fixes that. There are also several warnings:

os_os2.c
G:/trunk/mozilla/db/sqlite3/src/os_os2.c: In function `sqlite3OsOpenReadWrite':

G:/trunk/mozilla/db/sqlite3/src/os_os2.c:106: warning: comparison of unsigned expression < 0 is always false
G:/trunk/mozilla/db/sqlite3/src/os_os2.c:108: warning: comparison of unsigned expression < 0 is always false
G:/trunk/mozilla/db/sqlite3/src/os_os2.c: In function `sqlite3OsOpenExclusive':
G:/trunk/mozilla/db/sqlite3/src/os_os2.c:143: warning: comparison of unsigned expression < 0 is always false
G:/trunk/mozilla/db/sqlite3/src/os_os2.c: In function `sqlite3OsOpenReadOnly':
G:/trunk/mozilla/db/sqlite3/src/os_os2.c:165: warning: comparison of unsigned expression < 0 is always false
G:/trunk/mozilla/db/sqlite3/src/os_os2.c: In function `sqlite3OsTempFileName':
G:/trunk/mozilla/db/sqlite3/src/os_os2.c:225: warning: comparison between signed and unsigned
G:/trunk/mozilla/db/sqlite3/src/os_os2.c: In function `sqlite3OsSeek':
G:/trunk/mozilla/db/sqlite3/src/os_os2.c:313: warning: ISO C89 forbids mixed declarations and code


I don't know what sopen returns but I guess that "if( id->h<0 ){" should probably read "if( !id->h ){". This might fix the first four warnings. Don't understand the 5th one, but the last one just means that "long pos;" should be moved one line up.

Andy, can you have a look if my guesses make any sense? Did you manage to get the previous patches upstream?

Comment 18

[Andy Willis (abwillis)]

Attachment: sqlite 3.2.7 updates

Not entirely complete yet as there were some changes for troubleshooting that I haven't gotten to yet but this patch updates the OS/2 files to sqlite 3.2.7.
I still need to put patches together for sending upstream and should also create a patch for 3.2.8 for upstream as well.

Comment 19

[Andy Willis (abwillis)]

This last patch also does make one change that was just a mistake in my previous files:
+#if defined(THREADSAFE) && THREADSAFE
This line had been inside a comment so we were executing the threadsafe code in all instances.  I am a bit unsure what affect (if any) that will have overall but matches the other platforms.

Comment 20

[Andy Willis (abwillis)]

Disregard the previous comment about the threadsafe stuff, I was misseeing where the line was, it wasn't actually in the comment section but between two different comment sections.
Addressing the warnings.  If we changed it to if( !id->h ) then we would always execute the code in the if statement (which as it is working as is might break it).  The unix code uses an int where were are using a unsigned long.  I need to look at the original code from Andrew MacIntyre to see if this is code I copied from his patch or if this is code changes I did.  
Actually I just downloaded Andrew's files and this was code I had copied from his  patch.  I need to look up the return for sopen and see what a failure is represented by (apparently not a negative as we are currently looking for, that or we should be using an int instead of a unsigned long).
As the code is working I am guessing the unsigned long is correct (or I should be getting messages about the conversion I expect).

Comment 21

[Andy Willis (abwillis)]

OK, further investigation of the GCC and EMX headers both show that h should be an int instead of an unsigned long.  In fact that is the cause of the mixed declarations and code seems to be related to that as lseek is expecting to get an int but is getting an unsigned long.  I have made the change locally and am about to rebuild to make sure but that seems to be the cause of these warnings.

Comment 22

[Andy Willis (abwillis)]

Attachment: update sqlite to 3.2.7

This patch removes all the warnings from the OS/2 specific files, including a warning about comparing signed to unsigned where in a for loop an i was an int and sizeof is unsigned.

Comment 23

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Has this been submitted to upstream yet?  I don't see anything in the sqlite mailing list or bug DB.

Comment 24

[Andy Willis (abwillis)]

Not yet, I went to do it here while back but at the time couldn't post to the list and wasn't sure how to submit the patch.  I now have the information but haven't had the time.  Will try to do that this weekend.

Comment 25

[Peter Weilbacher]

Now, a few weeks later something else is broken. I get lots of errors again, starting with

X:/trunk/mozilla/db/sqlite3/src/os_os2.c:49: parse error before '.' token
X:/trunk/mozilla/db/sqlite3/src/os_os2.c:57: parse error before '.' token
X:/trunk/mozilla/db/sqlite3/src/os_os2.c:87: parse error before '.' token

This is because all the sqlite30sSTH functions are defined as sqlite3Os.xSTH, e.g. sqlite3Os.xDelete. Seems that the update to sqlite 3.3.3 screwed us again. The comment around line 130 io os.h suggests that we need to define all the functions for OS/2 in os_os2.h so that they have the same names as in os_os2.c. 

Andy, have you already tried to get that fixed or should I?

Comment 26

[Andy Willis (abwillis)]

3.3.3 has quite a few changes.  Daniel Kruse sent me the changes for 3.3.4 that he did and it builds fine but the browser crashes when I try to launch calendar.  I am trying to track down why it is crashing still.  I had successfully built with the 3.2.7 files so it is possible.

Comment 27

[Andy Willis (abwillis)]

Attachment: 3.3.4 updates

Here are the updates to 3.3.4 (Daniel Kruse did the work on this).  
I am crashing in btree.c with when this is built currently.

Comment 28

[Andy Willis (abwillis)]

When trying to use the calendar currently it crashes here:
shared-cache.
Changes in:
db/sqlite3/src/makefile.in
where 
DEFINES = -DSQLITE_ENABLE_REDEF_IO
were added to it so that
/storage/src/mozStorageAsyncIO.cpp
would work causes this.
SQLITE_ENABLE_REDEF_IO causes defines in os.h to change
http://lxr.mozilla.org/seamonkey/source/db/sqlite3/src/os.h#309

Comment 29

[Peter Weilbacher]

Mike Kaply checked in the SQLITE changes. Do you want to leave it open for the next round of SQLITE updates? Otherwise I would suggest to resolve as fixed and open a new bug when the need arises.

Comment 30

[Peter Weilbacher]

Oh, I had forgotten about the runtime crash. Unfortunately, I need a few hours more for a debug build, but I see a crash at
   c0000005
   125aac7d
   P1=00000001  P2=00000005  P3=XXXXXXXX  P4=XXXXXXXX
   [...]
   STRGCMPS.DLL 0001:0000ac7d

when trying to open Calendar. The corresponding .map file points to sqlite3AnalysisLoad but as in previous crashes that may not be entirely accurate.

Comment 31

[Peter Weilbacher]

Attachment: small extra patch for sqlite 3.3.4

OK, the actual crash occurred in sqlite3Os2ThreadSpecificData() is os_os2.c.

This small patch gets it working: Calendar starts and I can create tasks and events for which I also get alarms. :-)

Andy, would be great if you could get this upstream, too.

Comment 32

[Andy Willis (abwillis)]

Thanks, I have put hours into trying to track that down.  It was working on an existing database (after Mike's changes) but not for a new one.  I have it narrowed down to that code but it looped through that code 5 or 6 times before it crashed.
I too suggest closing this ticket and opening new ones needed.  I can submit the os_os2.c and os.h changes upstream but haven't worked out the configure changes, and as my build environment is broken (I can build Mozilla but configure for my non-Moz environment is horked).
BTW Peter, could you also attach your patch to get rid of the warnings?
Andy 

Comment 33

[Peter Weilbacher]

Attachment: all post 3.3.4 changes

OK, this combines the patch about fixing the warnings with the previous one.

In addition to the one if(!rc) instance that I had to change to get rid of the crash there are two more in sqlite3Os2IsDirWritable and os2CheckReservedLock that I find "suspicious" (they could be shorthand for rc==NO_ERROR but they could also be typos and really mean rc!=NO_ERROR) but they would not result in crashes. And as I don't really understand the code in detail I best left them alone.

Comment 34

[Andy Willis (abwillis)]

Comparing the code to os_unix.c I think the if ( !rc ) is correct in those other two locations.  The one in sqlite3Os2ThreadSpecificData() looks like a typo as the os_unix.c has if ( rc ) -- equivalent to your if (rc != NO_ERROR).  I ended up having to verify what I thought it was doing on #calendar to make sure I knew how the syntax evaluated.  I don't like the code this way because I have a hard time thinking in this way.  However, while I was verifying the syntax on #calendars they seemed to indicate this was the better syntax than an explicit formula to evaluate.  

Comment 35

[Peter Weilbacher]

The shorter syntax is certainly harder to maintain. Especially as the code is otherwise almost uncommented the longer version gives more explicit hints to what the functions in question return on success or failure. And the compiler should take care that in machine code both versions are equal. But it's really up to the maintainer (so probably Daniel) to decide what to use...

My feeling was that sqlite3Os2IsDirWritable() is supposed to return 1 on failure and 0 on success but it's really the other way around, so I think your analysis is correct.

Btw, I noticed that when I run the builds that use sqlite (with calendar or places) it generates hundreds of sqlite* files in my tempdir that seem to never get deleted.

Comment 36

[Mike Kaply [:mkaply]]

OK, all these changes are in. Let's mark this fixed and use new bugs.

Comment 37

[Peter Weilbacher]

Comment on attachment 191376 [details] [diff] [review]
More context in os.h diff

This review isn't necessary any more, SQLite is working on OS/2.