330340 - [BeOS] Build broken in sqlite

Status: RESOLVED FIXED

(Toolkit :: Storage, defect)

Description

[Doug Shelton]

User-Agent:       Mozilla/5.0 (BeOS; U; BeOS BePC; en-US; rv:1.9a1) Gecko/20060307 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (BeOS; U; BeOS BePC; en-US; rv:1.9a1) Gecko/20060307 Firefox/1.6a1

Sometime between 2006-03-07 and 2006-03-13, building broke in sqlite.  Console output in attachment.

Reproducible: Always

Comment 1

[Doug Shelton]

Attachment: Console output

Comment 2

[Sergei Dolgov]

We don't use pthreads in BeOS builds.
So, that may be configure/.mozconfig problem
or non-intelligent approach in sqlite code which treats BeOS as pure unix ignoring nspr existance and inventing wheel again

Comment 3

[Sergei Dolgov]

try to rollback patch for
https://bugzilla.mozilla.org/show_bug.cgi?id=328213
which defines THREADSAFE

Comment 4

[Doug Shelton]

(In reply to comment #3)
> try to rollback patch for
> https://bugzilla.mozilla.org/show_bug.cgi?id=328213
> which defines THREADSAFE
> 
I noticed this patch hit the trunk on 2006-02-22.  I completed a trunk update/build on 2006-03-07.  This patch touches quite a few files and may be risky for me to roll back.  Based on the timing of my last OK build, do you still think this is the issue, fyysik?

Comment 5

[Sergei Dolgov]

Oops!
I meant 
https://bugzilla.mozilla.org/show_bug.cgi?id=328598
and very little patch for that
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/db/sqlite3/src&command=DIFF_FRAMESET&file=Makefile.in&rev1=1.13&rev2=1.14&root=/cvsroot

Comment 6

[Doug Shelton]

(In reply to comment #5)
I've reversed the changes in this bug and am rebuilding now.

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

sqlite is a 3rd party library and presumably doesn't want an NSPR dependency...

Comment 8

[Doug Shelton]

confirming fyysik's suspicion:  backing out 328598 allowed build to complete.  So maybe 2 approaches:  permanently disable threading for sqlite under BeOS, or find a way to use bthreads as I understand we do elsewhere in the code.

Comment 9

[Sergei Dolgov]

Per Comment 7:
Now I feel myself but uncomfortable about most correct way to fix this and in future similar 3rd party components.
Should we propose fix to sqlite team introducing native threads?
Add pthreads (quite flacky) as dependency/requirement for BeZilla?
Add OS check only in affected Makefile.In to disable threads for BeOS?

Comment 10

[Doug Shelton]

Maybe we have short-term and long-term goals.  Short-term, put a dependency in Makefile.in so sqlite doesn't use threads on BeOS.  Long-term, work with sqlite team to add native thread support.  If support of pthreads under BeOS is risky, better so sacrafice a small amount of performance for reliability, IMO.

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

um... if mozilla uses sqlite from multiple threads (which I assume it does, given that THREADSAFE define change), then I would really not recommend compiling sqlite in a non-threadsafe way for beos...

Comment 12

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

(In reply to comment #7)
> sqlite is a 3rd party library and presumably doesn't want an NSPR dependency...

Yeah, though sqlite's os stuff is abstracted away -- an os_nspr.c for our own use would be very doable, I think; just hasn't been a need for it yet.  Patches accepted, etc.

Comment 13

[Doug Shelton]

(In reply to comment #12)
>  Patches accepted, etc.
> 
As I'm mostly just build/test for the Bezilla team, I'll have to defer to fyysik, biesi and others for the actual patch, but I'm ready and willing to test/validate anything the team generates.

Comment 14

[Doug Shelton]

(In reply to comment #5)
I did a little more testing and found I only needed to reverse the change to sqlite3/src/Makefile.in.  Firefox builds OK without reversing the change to nsNavHistory.cpp

Comment 15

[Doug Shelton]

BeOS building is broken until we can resolve this issue.  Biesi/fyysik, is there any way to tell if Firefox code actually requires the THREADSAFE parameter?  Is there a simple way to add bthreads capability to sqlite?

Comment 16

[Sergei Dolgov]

Per comment 15:
Second question consists of two - 1)is it easy to add bthreads support code 2)How fast and easy is to put it in sqlite source tree.


First question. As you now have somewhere working build with disabled pthreads,
look if you have in your build any issue described in bug report.
https://bugzilla.mozilla.org/show_bug.cgi?id=328598

If it isn't the case, i think we can exclude that DEFINES in Makefile.in for BeOS.

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #15)
> BeOS building is broken until we can resolve this issue.  Biesi/fyysik, is
> there any way to tell if Firefox code actually requires the THREADSAFE
> parameter?

I don't know the answer to that...

Comment 18

[Doug Shelton]

(In reply to comment #16)
> First question. As you now have somewhere working build with disabled pthreads,
> look if you have in your build any issue described in bug report.
> https://bugzilla.mozilla.org/show_bug.cgi?id=328598
> 
> If it isn't the case, i think we can exclude that DEFINES in Makefile.in for
> BeOS.
> 
The slow shutdown problems seem to occur on systems with very large (10-12MB) history.sqlite files.  I checked; mine is only 849K, so I can't say for sure whether BeOS will be impacted by the issues described in the bug.  I think perhaps not, though, because of BFS's relatively good performance.

I'll try increase the size of my bookmark/history database to the ludicrous sizes mentioned in the bug and see if my shutdown is still slow.

Comment 19

[Brett Wilson]

(In reply to comment #16)

> First question. As you now have somewhere working build with disabled pthreads,
> look if you have in your build any issue described in bug report.
> https://bugzilla.mozilla.org/show_bug.cgi?id=328598

The slow shutdown is unrelated to whether you have a multithreaded enabled sqlite build. It only had to do with 0-fill when history was expired.

The MozStorageAsyncIO is also unrelated to whether you have a multithreaded-enabled sqlite build, it uses Mozilla threads.

Comment 20

[Sergei Dolgov]

Attachment: patch for Makefile.in

Doug, you can test if given patch (with original nsNavHistory.cpp) does the deal, it means - allows BeOS version to build and don't cause instability in history/places.

Comment 21

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 215426 [details] [diff] [review]
patch for Makefile.in

do you mean ifneq? and what about the SQLITE_ENABLE_REDEF_IO define?

Comment 22

[Sergei Dolgov]

per Comment 21:
Yes, you are right, ifneq.
Sorry. Will submit another patch for Doug's convinience.

Unfortunately, I cannot comment in given situation our need for overiding IO functions, probably is safer to change patch to something like that
DEFINES = -DSQLITE_ENABLE_REDEF_IO 
infeq ($(OS_ARCH),BeOS)
DEFINES -DTHREADSAFE=1
endif

Unfortunately I'm leaving again for two weeks to Sweden, so cannot experiment with this problem myself.

Comment 23

[Doug Shelton]

(In reply to comment #22)
> per Comment 21:
> Yes, you are right, ifneq.
> Sorry. Will submit another patch for Doug's convinience.
> 
> Unfortunately, I cannot comment in given situation our need for overiding IO
> functions, probably is safer to change patch to something like that
> DEFINES = -DSQLITE_ENABLE_REDEF_IO 
> infeq ($(OS_ARCH),BeOS)
> DEFINES -DTHREADSAFE=1
> endif
> 
> Unfortunately I'm leaving again for two weeks to Sweden, so cannot experiment
> with this problem myself.
> 

no need to submit another patch... I think I can correct and test.

Comment 24

[Doug Shelton]

Attachment: patch - does not set THREADSAFE under BeOS

This patch allows BeOS to build.  If I understand the syntax correctly, it will set THREADSAFE on all other platforms.

Comment 25

[Sergei Dolgov]

Doug, maybe it needs more testing?

Comment 26

[Doug Shelton]

(In reply to comment #25)
> Doug, maybe it needs more testing?
> 
Probably.  It seems the declaration is not needed for now, though.  See https://bugzilla.mozilla.org/show_bug.cgi?id=328598#c30  The greater question is what will happen if/when "places" expects to use threads in the future.

Comment 27

[Sergei Dolgov]

Ok, I see.
But
1) I think it is good to let ifneq BeOS to be in code, so in future developers don't need special kind of reminder, like bustage, that there were problems.
2)Form of patch.
I'm curios if
+DEFINES = -DSQLITE_ENABLE_REDEF_IO
+
+ifneq ($(OS_ARCH),BeOS)
 DEFINES = -DTHREADSAFE=1
+endif
will work and is more correct,
because in your version for non-BeOS platform we go to define
DEFINES = -DSQLITE_ENABLE_REDEF_IO twice.

possible forms are also
ifneq ($(OS_ARCH),BeOS)
DEFINES = -DSQLITE_ENABLE_REDEF_IO -DTHREADSAFE=1
else
DEFINES = -DSQLITE_ENABLE_REDEF_IO
endif

or
ifeq ($(OS_ARCH),BeOS)
DEFINES = -DSQLITE_ENABLE_REDEF_IO
else
DEFINES = -DSQLITE_ENABLE_REDEF_IO -DTHREADSAFE=1
endif

Comment 28

[Doug Shelton]

(In reply to comment #27)
> 2)Form of patch.
>
Form of patch is simply what happens when a non-dev tries to become a dev.  ;)  I was unsure if the second define added to the first, or worked in place of it.  I like your if/else construct best, as it makes very clear our intention.  Let me revise, test and resubmit.

Comment 29

[Doug Shelton]

Attachment: simplified, more clear syntax

Comment 30

[Sergei Dolgov]

Comment on attachment 215527 [details] [diff] [review]
simplified, more clear syntax

looks more correct now

Comment 31

[Brett Wilson]

I would suggest just removing THREADSAFE for all platforms. Multithreaded access doesn't work now anyway and I'm not sure why I put it in. If we actually make it multithreaded, we can put in the conditional code you suggest here.

Comment 32

[Doug Shelton]

(In reply to comment #31)
> I would suggest just removing THREADSAFE for all platforms. Multithreaded
> access doesn't work now anyway and I'm not sure why I put it in. If we actually
> make it multithreaded, we can put in the conditional code you suggest here.
> 
I'm good either way.  One advantage of leaving it this way:  we'll leave a record and thus ensure we remember the BeOS restriction in the future.  

Comment 33

[Doug Shelton]

(In reply to comment #31)
> I would suggest just removing THREADSAFE for all platforms. Multithreaded
> access doesn't work now anyway and I'm not sure why I put it in. If we actually
> make it multithreaded, we can put in the conditional code you suggest here.
> 

Can we commit the patch as is?  As I've thought about it, I'd rather leave a trail about threadsafe so it doesn't come back to haunt us later.

Brett, I don't have the ability to commit code.  I'd greatly appreciate your committing this change if you agree.  We can then close this bug and let BeOS build from trunk again.

Comment 34

[Doug Shelton]

biesi, fyssik is out for another week and I don't have the ability to commit code changes.  If you have a moment to commit this change, we'd greatly appreciate your help.

Comment 35

[Christian :Biesinger (don't email me, ping me on IRC)]

I do not want to commit this. defining threadsafe only on some platforms is (as opposed to either defining it nowhere or defining it everywhere), in my opinion, the wrong thing to do. please find someone else.

Comment 36

[Brett Wilson]

Comment on attachment 215527 [details] [diff] [review]
simplified, more clear syntax

Sorry, I was thinking about this last night and I think THREADSAFE is a bad idea. My enabling it was a mistake. As I suggested above in comment 31, threadsafe should be removed from all platforms.

Comment 37

[Doug Shelton]

(In reply to comment #36)
> (From update of attachment 215527 [details] [diff] [review] [edit])
> Sorry, I was thinking about this last night and I think THREADSAFE is a bad
> idea. My enabling it was a mistake. As I suggested above in comment 31,
> threadsafe should be removed from all platforms.
> 
No problem.  I'll change to remove for all platforms and submit and updated patch for review.

Comment 38

[Doug Shelton]

Attachment: one-line change removes threadsafe option for all platforms

BeOS builds OK with this change.  If it passes reviews, I'd appreciate it if either fyysik or Brett could update the trunk.  I don't have the ability to commit changes.

Comment 39

[Brett Wilson]

Comment on attachment 216474 [details] [diff] [review]
one-line change removes threadsafe option for all platforms

Looks good, I'll check it in soon.

Comment 40

[Doug Shelton]

(In reply to comment #39)
> (From update of attachment 216474 [details] [diff] [review] [edit])
> Looks good, I'll check it in soon.
> 
As soon as possible, please. 

Comment 41

[Doug Shelton]

checkin requested after review by fyysik or tqh

Comment 42

[tqh]

Comment on attachment 216474 [details] [diff] [review]
one-line change removes threadsafe option for all platforms

r=[e-mail]

Comment 43

[Christian :Biesinger (don't email me, ping me on IRC)]

Checking in db/sqlite3/src/Makefile.in;
/cvsroot/mozilla/db/sqlite3/src/Makefile.in,v  <--  Makefile.in
new revision: 1.15; previous revision: 1.14
done

Comment 44

[Christian :Biesinger (don't email me, ping me on IRC)]

is this needed on branches too?

Comment 45

[Doug Shelton]

(In reply to comment #44)
> is this needed on branches too?
> 
I only build from trunk, so it's hard to say.  BrettW would probably know whether the same issue exists in the branches.  Thanks for the commit, Biesi!

Comment 46

[Brett Wilson]

Yup, this should be on branch as well. Just checked in.

Comment 47

[Brett Wilson]

Threadsafe is going to get turned on again in bug 336314.

Comment 48

[Sergei Dolgov]

Will it be possible to add ifneq()/if !defined  for BeOS in that coming bug?

Comment 49

[Brett Wilson]

(In reply to comment #48)
> Will it be possible to add ifneq()/if !defined  for BeOS in that coming bug?

No. If BeOS can't handle threadsafe, I think they should either fix sqlite or not use sqlite. We make threadsafety guarantees in the storage service documentation. Having it randomly corrupt your database on BeOS when you follow the instructions is a recipe for disaster for BeOS. The only service that will use the storage service in Firefox 2.0 requires this threadsafety.

Comment 50

[Sergei Dolgov]

There may be another problem with sqlite and BeOS in future.
Locking.
This is the reason, why still no official port of SAMBA for BeOS.
Parallel "simultaneous" access to files is quite normal for Be File System, as far as i understand, so, inspite all tricks and workarounds in locking implementation, "real locking" tests in configure utilities usually fail, and "hard" locking, like that in Windows, is still not-implemented.

Comment 51

[Brett Wilson]

(In reply to comment #50)
> There may be another problem with sqlite and BeOS in future.
> Locking.
> This is the reason, why still no official port of SAMBA for BeOS.
> Parallel "simultaneous" access to files is quite normal for Be File System, as
> far as i understand, so, inspite all tricks and workarounds in locking
> implementation, "real locking" tests in configure utilities usually fail, and
> "hard" locking, like that in Windows, is still not-implemented.

Our implementation does not depend on file locking. Currently, the locks don't do anything because with our caching system, everything will break if > 1 process accesses it anyway.

Comment 52

[Sergei Dolgov]

We have now some native version with bthreads for 3.3.5.
includes beos_os.c and modified makefile.
But no autoconf patches yet.
So there is question which way to go now - fix it locally for Mozilla only or try (may take long time) to land changes into original sqlite cvs

Comment 53

[Brett Wilson]

(In reply to comment #52)
> We have now some native version with bthreads for 3.3.5.
> includes beos_os.c and modified makefile.
> But no autoconf patches yet.
> So there is question which way to go now - fix it locally for Mozilla only or
> try (may take long time) to land changes into original sqlite cvs

We're going to upgrade to the latest verion of sqlite in the next few weeks, so don't check any changes in until then unless you want to redo them. I would certianly offer your changes to D.R. Hipp first, though he might not want them.

Comment 54

[Doug Shelton]

(In reply to comment #53)
> 
> We're going to upgrade to the latest verion of sqlite ...
> 

Latest in tarball form on sqlite.com is 3.3.5.  Is there a later version in sqlite CVS, and if so, what is the targeted version to be included in Mozilla?

Comment 55

[Brett Wilson]

I guess that's it.

Comment 56

[tqh]

Doug, could you upload a patch that we can use until the sqlite upgrade is done?

Comment 57

[Doug Shelton]

(In reply to comment #56)
> Doug, could you upload a patch that we can use until the sqlite upgrade is
> done?
> 
Patch file is pretty substantial.  It is cleaner to do a wholesale replacement of the sqlite3 subdirectory.  I've uploaded an archive of mine in this bug.  I apologize for the binary.

Comment 58

[Doug Shelton]

Uploaded archive is available here: 
http://www.sheltonfamily.org/firefox_test/sqlite3.zip

Comment 59

[Brett Wilson]

Doug, upgrading sqlite will be a continuous process. We need a patch for these future upgrades. Just copying your big directory is not a solution.

Comment 60

[Doug Shelton]

(In reply to comment #59)
> Doug, upgrading sqlite will be a continuous process. We need a patch for these
> future upgrades. Just copying your big directory is not a solution.
> 
Brett, thanks, I understand.  I've already placed a patch with our changes in the tracker bug for 3.3.5.  Please see  https://bugzilla.mozilla.org/show_bug.cgi?id=330340  From our discussions, I understood having BeOS-specific changes to sqlite 3.3.5 would be preferred over a large patch diffing all changes between current mozilla sqlite and my version using 3.3.5 with BeOS changes. 

This bug is already closed, though I suppose it should be reopened and linked to bug 338155, which will fix the problem permanently (and hopefully soon?)  In the meantime, those few BeOS devs working with mozilla code can get a working copy.

Comment 61

[Doug Shelton]

reopening bug.  changes elsewhere in code now mandate THREADSAFE=1.  Will be resolved when migration to sqlite 3.3.5 including BeOS changes is made in bug 338155.

Comment 62

[Doug Shelton]

Attachment: modifications to CVS sqlite 3.3.5 to add BeOS.

To add BeOS support:
 - Apply this patch to /mozilla/db/sqlite3/src.  
 - Copy os_beos.c to /mozilla/db/sqlite3/src.

Comment 63

[Doug Shelton]

Attachment: beos-specific code for sqlite3

Comment 64

[Sergei Dolgov]

Comment on attachment 222968 [details] [diff] [review]
modifications to CVS sqlite 3.3.5 to add BeOS.

for me looks like
define OS_BEOS 0 (2 places!)
elif defined(__BEOS__)
strings identation is wrong

Comment 65

[Doug Shelton]

(In reply to comment #64)
> (From update of attachment 222968 [details] [diff] [review] [edit])
> for me looks like
> define OS_BEOS 0 (2 places!)
> elif defined(__BEOS__)
> strings identation is wrong
> 
The multiple define is odd, I agree, but consistent with the code already present in sqlite3 on which it's based.  The defines themselves are present in the code 3 times.  OS_WIN, OS_UNIX and OS_BEOS only get defined once in a series of if/elif statements.

Indentation is ugly all through the original sqlite3 code - difficult to read, at least for me.  I'm updating the attachments with what (I hope) will pass review. 

Comment 66

[Doug Shelton]

Attachment: updated patch

refactored code to address sergei's concerns

Comment 67

[Doug Shelton]

Attachment: beos-specific code for sqlite3 - refactored

Comment 68

[Sergei Dolgov]

Comment on attachment 223036 [details] [diff] [review]
updated patch

tigerdog, don't rush.
Contact me at IRC before submitting new file:)

Comment 69

[tqh]

Comment on attachment 223037 [details]
beos-specific code for sqlite3 - refactored

Let' make it readable.

Comment 70

[tqh]

Comment on attachment 223037 [details]
beos-specific code for sqlite3 - refactored

oops.

Comment 71

[Doug Shelton]

Attachment: updated again for use with current CVS (includes OS2)

previous version was taken from CVS before landing of OS2 fix from bug 333835.

Comment 72

[Sergei Dolgov]

Comment on attachment 223066 [details] [diff] [review]
updated again for use with current CVS (includes  OS2)

first r=sergei_d
applies and builds cleanly.

second review request

Comment 73

[Sergei Dolgov]

Comment on attachment 223037 [details]
beos-specific code for sqlite3 - refactored

tigerdog,
1)put identation back to original state just here, in os_beos.c
2) remove "/usr" line from code, there is no such folder in BeOS.
Remaining looks ok  for start.

Comment 74

[Doug Shelton]

Attachment: os_beos.c BeOS-specific code for sqlite 3

removed ref to /usr and unneeded refactoring.

Comment 75

[Sergei Dolgov]

Comment on attachment 223920 [details]
os_beos.c  BeOS-specific code for sqlite 3

pure platform-specific (BeOS) file, but asking second review anyway.

Comment 76

[Brett Wilson]

Comment on attachment 223066 [details] [diff] [review]
updated again for use with current CVS (includes  OS2)

The patch to the shared files looks fine.

Comment 77

[Brett Wilson]

Comment on attachment 223920 [details]
os_beos.c  BeOS-specific code for sqlite 3

I didn't really look at os_beos.c, but since you're the only people that use it, you can take responsibility for it. :)

Comment 78

[Sergei Dolgov]

Checking in mozilla/db/sqlite3/src/Makefile.in;
/cvsroot/mozilla/db/sqlite3/src/Makefile.in,v  <--  Makefile.in
new revision: 1.18; previous revision: 1.17
done
Checking in mozilla/db/sqlite3/src/os.h;
/cvsroot/mozilla/db/sqlite3/src/os.h,v  <--  os.h
new revision: 1.10; previous revision: 1.9
done
RCS file: /cvsroot/mozilla/db/sqlite3/src/os_beos.c,v
done
Checking in mozilla/db/sqlite3/src/os_beos.c;
/cvsroot/mozilla/db/sqlite3/src/os_beos.c,v  <--  os_beos.c
initial revision: 1.1
done 

Comment 79

[Sergei Dolgov]

patches landed for trunk, wondering if we are interested in branch?

Comment 80

[Doug Shelton]

reopening bug until patch can be landed in 1.8.1 branch.  BeOS cannot build 1.8.1 without this.

Comment 81

[Niels Reedijk]

I'll see if I can bug someone that wants to commit this...

Comment 82

[Niels Reedijk]

Remove the fixed1.8.1 keyword: it hasn't been fixed yet...

Comment 83

[Doug Shelton]

thanks, neils.  I thought it had been committed but just pulled a clean tree and discovered it wasn't.  Welcome back!

Comment 84

[Martijn Wargers (dead)]

Checking in os.h;
/cvsroot/mozilla/db/sqlite3/src/os.h,v  <--  os.h
new revision: 1.2.8.6; previous revision: 1.2.8.5
done
Checking in Makefile.in;
/cvsroot/mozilla/db/sqlite3/src/Makefile.in,v  <--  Makefile.in
new revision: 1.7.2.10; previous revision: 1.7.2.9
done
Checking in os_beos.c;
/cvsroot/mozilla/db/sqlite3/src/os_beos.c,v  <--  os_beos.c
new revision: 1.1.4.1; previous revision: 1.1
done

Fixed on 1.8.1 branch.