Closed Bug 494826 Opened 11 years ago Closed 11 years ago

Compile SQLite with SQLITE_DEBUG defined

Categories

(Toolkit :: Storage, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: sdwilsh, Assigned: sdwilsh)

References

Details

Attachments

(1 file, 6 obsolete files)

This will point out when we are breaking assumptions that SQLite makes in debug builds.  Certainly do not want this done in release builds though.
Blocks: 494828
Attached patch v1.0 (obsolete) — Splinter Review
I pushed this to the try server, but this does seem to do what I'd expect locally on my mac (in terms of preprocessing it).
Assignee: nobody → sdwilsh
Status: NEW → ASSIGNED
Attachment #379806 - Flags: review?(ted.mielczarek)
Whiteboard: [needs review ted]
Something is wrong with this patch, but I'm not sure what:
make[5]: *** No rule to make target `../../../dist/bin/sqlite.def', needed by `sqlite3.dll'.  Stop.
make[4]: *** [libs] Error 2
make[3]: *** [libs_tier_gecko] Error 2
make[2]: *** [tier_gecko] Error 2
make[1]: Leaving directory `/e/builds/sendchange-slave/sendchange-win32-hg/build/objdir'
make[1]: *** [default] Error 2
make: *** [build] Error 2
I strongly suspect that libs:: is to late for the def file preprocessing.
Attached patch v1.1 (obsolete) — Splinter Review
This one actually passes the try server.
Attachment #379806 - Attachment is obsolete: true
Attachment #379925 - Flags: review?(ted.mielczarek)
Attachment #379806 - Flags: review?(ted.mielczarek)
Not sure if I need to add sqlite.def to GARBAGE now either.
Depends on: 494124
Attachment #379925 - Flags: review?(ted.mielczarek) → review-
Comment on attachment 379925 [details] [diff] [review]
v1.1

+DEFFILE = $(FINAL_TARGET)/sqlite.def

You don't want FINAL_TARGET, that's something like $(DIST)/bin:
http://mxr.mozilla.org/mozilla-central/source/config/config.mk#88

I'd use $(CURDIR), which is the directory you're in in the objdir.

+export::
+	@$(PYTHON) $(topsrcdir)/config/Preprocessor.py $(DEFINES) \
+	  $(srcdir)/sqlite.def > $(FINAL_TARGET)/sqlite.def
+

The way you have this written will cause this to be executed every time this directory gets built. I would instead do something like:

$(LIBRARY): $(CURDIR)/sqlite.def

$(CURDIR)/sqlite.def: $(srcdir)/sqlite.def:
  ...

This way make will run your rule only if sqlite.def doesn't exist in the objdir, or if the copy in the srcdir is newer. (In which case it will relink the library as well.)
Whiteboard: [needs review ted]
Attached patch v1.2 (obsolete) — Splinter Review
Per comments - I pushed this to the try server to make sure it works.
Attachment #379925 - Attachment is obsolete: true
Attachment #382543 - Flags: review?(ted.mielczarek)
Whiteboard: [needs review ted]
This failed on try server - the file didn't get copied over in the object directory:
LINK : fatal error LNK1104: cannot open file '/e/builds/slave/sendchange-win32-hg/build/objdir/db/sqlite3/src/sqlite.def'

Not really sure why :/
Attachment #382543 - Flags: review?(ted.mielczarek)
Whiteboard: [needs review ted] → [needs new patch]
No longer depends on: 494124
Got the same error when changing $(LIBRARY) to exports::

Still need to figure out what to do here.
Can you paste more of the log, specifically the linker commandline that's failing? From that error message, it looks like an MSYS path is making its way into link.exe, which clearly doesn't know what to do with it.
Sure thing:
link -NOLOGO -DLL -OUT:sqlite3.dll -PDB:sqlite3.pdb -SUBSYSTEM:WINDOWS  sqlite3.obj    sqlite.res -NXCOMPAT -SAFESEH -DYNAMICBASE  -MANIFEST:NO -LIBPATH:"e:/builds/slave/sendchange-win32-hg/build/objdir/memory/jemalloc/crtsrc/build/intel" -NODEFAULTLIB:msvcrt -NODEFAULTLIB:msvcrtd -DEFAULTLIB:mozcrt19 -DEBUG -OPT:REF -OPT:nowin98 -DEF:/e/builds/slave/sendchange-win32-hg/build/objdir/db/sqlite3/src/sqlite.def    kernel32.lib user32.lib gdi32.lib winmm.lib wsock32.lib advapi32.lib   
LINK : fatal error LNK1104: cannot open file '/e/builds/slave/sendchange-win32-hg/build/objdir/db/sqlite3/src/sqlite.def'
(In reply to comment #11)
> -DEF:/e/builds/slave/sendchange-win32-hg/build/objdir/db/sqlite3/src/sqlite.def

That's the problem. MSYS doesn't do the path translation there because there's no space before that path. :-/ You can try changing $(CURDIR)/sqlite3.def to ./sqlite3.def, and see if that fixes it.
Just pushed the change suggested in comment 12 to the try server.  Let's hope it works.
Sadly, no luck.  I'm going to try no ./ now.

link -NOLOGO -DLL -OUT:sqlite3.dll -PDB:sqlite3.pdb -SUBSYSTEM:WINDOWS  sqlite3.obj    sqlite.res -NXCOMPAT -SAFESEH -DYNAMICBASE  -MANIFEST:NO -LIBPATH:"e:/builds/slave/sendchange-win32-hg/build/objdir/memory/jemalloc/crtsrc/build/intel" -NODEFAULTLIB:msvcrt -NODEFAULTLIB:msvcrtd -DEFAULTLIB:mozcrt19 -DEBUG -OPT:REF -OPT:nowin98 -DEF:./sqlite.def    kernel32.lib user32.lib gdi32.lib winmm.lib wsock32.lib advapi32.lib   
LINK : fatal error LNK1104: cannot open file './sqlite.def'
Hm, that seems broken.
that did not work either - any ideas ted?
Attached patch v1.3 (obsolete) — Splinter Review
Different approach.  Sadly, it means we add a file to the srcdir, but I think it's OK (we already actually do this).
Attachment #382543 - Attachment is obsolete: true
Attachment #387012 - Flags: review?(ted.mielczarek)
Comment on attachment 387012 [details] [diff] [review]
v1.3

This one passes the try server!  Yay!
Whiteboard: [needs new patch] → [needs review ted]
Comment on attachment 387012 [details] [diff] [review]
v1.3

>diff --git a/db/sqlite3/src/Makefile.in b/db/sqlite3/src/Makefile.in

>+# We have to preprocess our def file because we need different symbols in debug
>+# builds exposed that are not built in non-debug builds.
>+$(DEFFILE_NAME): sqlite.def
>+	@$(PYTHON) $(topsrcdir)/config/Preprocessor.py $(DEFINES) \
>+	  $(srcdir)/sqlite.def > $(srcdir)/$(DEFFILE_NAME)

Writing a file to the srcdir during a build is not acceptable. Not only might this cause conflicts for people using multiple objdirs, but if you have a readonly srcdir, such as when building from a readonly mount, this operation will fail completely. The file need to be in the objdir.

And, use $@ for the target and $< for the source file instead of including the full name in the rule again.
Attachment #387012 - Flags: review?(ted.mielczarek) → review-
I guess I'll need to figure out how to get a win_curdir variable since msys doesn't do the path filtering otherwise.
Whiteboard: [needs review ted] → [needs new patch]
I've filed http://www.sqlite.org/cvstrac/tktview?tn=3983 in order to use __dllspec(import) and __dllspec(export) on SQLITE_API, so we don't have to worry about the def file anymore.  I'm not sure how much this is going to hurt OS2 though...
Attached patch v2.0 (obsolete) — Splinter Review
un-tested new approach

The try server doesn't like this, but it looks like infra issues as opposed to my patch.  Will work on this more tomorrow.
Attachment #387012 - Attachment is obsolete: true
Shawn, thanks for the heads-up. GCC on OS/2 does support those decorations, too, and they are used in other parts of Mozilla code. So I hope that is not going to hurt us at all, on the contrary. I will give it a try.
Note that I ended up not going that way with the latest patch because SQLite was making it a bit difficult.
Attached patch v3.0 (obsolete) — Splinter Review
This one seemed to work well locally.  Pushed to try server to see how it goes there.
Attachment #389596 - Attachment is obsolete: true
Attachment #389768 - Flags: review?(ted.mielczarek)
Whiteboard: [needs new patch] → [needs review ted]
Attachment #389768 - Flags: review?(benjamin)
Comment on attachment 389768 [details] [diff] [review]
v3.0

ted won't be able to get to this until next week at the earliest, so if bsmedberg can get to this first, that'd be great (this bug blocks other bugs that end up blocking bug 455555).
Comment on attachment 389768 [details] [diff] [review]
v3.0

Could you not directly use DEFFILE = $(CURDIR)/sqlite-processed.def?

For this patch not to break OS/2 you have to change $(srcdir)/sqlite.def in ADD_TO_DEF_FILE to $(CURDIR)/sqlite-processed.def, too. I hope that the pre-processing is run on OS/2 before that, still have to try that.
(In reply to comment #27)
> (From update of attachment 389768 [details] [diff] [review])
> Could you not directly use DEFFILE = $(CURDIR)/sqlite-processed.def?
Are you complaining about the use of $(DEFFILE_NAME)?  I'm using that because I didn't want to copy and paste the name in multiple places (and in fact, I had a typo in one place in one of my earlier attempts).

> For this patch not to break OS/2 you have to change $(srcdir)/sqlite.def in
> ADD_TO_DEF_FILE to $(CURDIR)/sqlite-processed.def, too. I hope that the
> pre-processing is run on OS/2 before that, still have to try that.
I can make that change before I land.
Comment on attachment 389768 [details] [diff] [review]
v3.0

You also want to add $(DEFFILE) to GARBAGE. Thanks for sticking through this and getting it done right!
Attachment #389768 - Flags: review?(ted.mielczarek) → review+
(In reply to comment #28)
> (In reply to comment #27)
> > (From update of attachment 389768 [details] [diff] [review] [details])
> > Could you not directly use DEFFILE = $(CURDIR)/sqlite-processed.def?
> Are you complaining about the use of $(DEFFILE_NAME)?  I'm using that because I
> didn't want to copy and paste the name in multiple places

OK, but you are always using $(CURDIR)/$(DEFFILE_NAME) together, so you could as well use $(DEFFILE) instead (and then you don't need DEFFILE_NAME at all). Not a big deal, though.

> > For this patch not to break OS/2 you have to change $(srcdir)/sqlite.def in
> > ADD_TO_DEF_FILE to $(CURDIR)/sqlite-processed.def, too. I hope that the
> > pre-processing is run on OS/2 before that, still have to try that.
> I can make that change before I land.

OK, I was wrong before, of course the pre-processor is never run on OS/2 because it's inside the WIN block (and because our .def file is DEF_FILE). What seems to work is to replace the whole ADD_TO_DEF_FILE line with

ADD_TO_DEF_FILE = $(PYTHON) $(topsrcdir)/config/Preprocessor.py $(DEFINES) \
        $(srcdir)/sqlite.def | sed -e '1,/^EXPORTS$$/ d' -e 's,sqlite3,_\0,' \
        -e 's,\ DATA.*$$,,' >> $(DEF_FILE)

so that the preprocessor directly writes to the correct output file. I don't like the duplication this introduces but I don't see a better way.
Whiteboard: [needs review ted]
Attached patch v3.1Splinter Review
Addresses review comments.  I've pushed this to the try server to make sure it builds before I land this on mozilla-central.
Attachment #389768 - Attachment is obsolete: true
Attachment #389768 - Flags: review?(benjamin)
http://hg.mozilla.org/mozilla-central/rev/48a8978d67f6
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-places]
Target Milestone: --- → mozilla1.9.2a1
Hm, this seems to have broken my minefield debug build...

link -NOLOGO -DLL -OUT:sqlite3.dll -PDB:sqlite3.pdb -SUBSYSTEM:WINDOWS  sqlite3.obj    sqlite.res -MANIFESTUAC:NO -NXCOMPAT -SAFESEH -DYNAMICBASE  -DEBUG -DEBUGTYPE:CV       -DEF:D:/mozilla-build/msys/comm-centralminefield-debug/db/sqlite3/src/sqlite-processed.def   kernel32.lib user32.lib gdi32.lib winmm.lib wsock32.lib advapi32.lib

errors out with

LINK : fatal error LNK1104: cannot open file 'D:/mozilla-build/msys/comm-centralminefield-debug/db/sqlite3/src/sqlite-processed.def'

There should be a / between |comm-central| and |minefield-debug|, but there isn't, which causes things to break. The file sqlite-processed.def does exist, though, in D:\mozilla-build\msys\comm-central\minefield-debug\db\sqlite3\src.

My mozconfig is:
mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/../../minefield-debug/
ac_add_options --enable-application=browser
ac_add_options --disable-optimize
ac_add_options --enable-debug
ac_add_options --with-windows-version=601

and I'm building from within /comm-central/src/.mozilla-trunk, hence the double .. -- I've tried other paths, including absolute ones, and it doesn't seem to make a difference.
Depends on: 506576
Shawn, seeing the thread about missing symbols in mozilla.dev.builds I now wonder why you chose to use #ifdef SQLITE_DEBUG instead of #ifdef DEBUG in sqlite.def? Is that already defined when the preprocessor is called? (I don't have Windows so I didn't test.)

sid0: you might also want to post to mozilla.dev.builds with this kind of problems. You should check the paths in your autoconf.mk to see which one is wrong. (To me this looks like your make version is broken that it cannot correctly resolve the CURDIR variable...)
(In reply to comment #35)
> sid0: you might also want to post to mozilla.dev.builds with this kind of
> problems. You should check the paths in your autoconf.mk to see which one is
> wrong. (To me this looks like your make version is broken that it cannot
> correctly resolve the CURDIR variable...)

Bug 506576 was filed and fixed ;)
It might not actually be defined...

If it is a problem, someone should file a new bug on it
You need to log in before you can comment on or make changes to this bug.