depends: Do not override `CFLAGS` when building SQLite with `DEBUG=1` #29287

hebasto · 2024-01-20T11:53:50Z

The --enable-debug configure option for the SQLite package does two things:

#-----------------------------------------------------------------------
#   --enable-debug
#
AC_ARG_ENABLE(debug, [AS_HELP_STRING(
  [--enable-debug], [build with debugging features enabled [default=no]])], 
  [], [])
AC_MSG_CHECKING([Build type])
if test x"$enable_debug" = "xyes"; then
  BUILD_CFLAGS="$BUILD_CFLAGS -DSQLITE_DEBUG -DSQLITE_ENABLE_SELECTTRACE -DSQLITE_ENABLE_WHERETRACE"
  CFLAGS="-g -O0"
  AC_MSG_RESULT([debug])
else
  AC_MSG_RESULT([release])
fi
#-----------------------------------------------------------------------

It adds three preprocessor definitions and overrides CFLAGS with "-g -O0". The latter breaks the user's ability to provide sanitizer and LTO flags.

This PR might be especially useful for OSS-Fuzz where DEBUG=1 has been used since google/oss-fuzz#10503.

Also it makes a workaround for building SQLite for 32-bit unneeded. For details, please refer to https://github.com/hebasto/oss-fuzz/tree/240120-sqlite.

Changes in #29282 might not be strictly required now. However, I consider them an improvement.

DrahtBot · 2024-01-20T11:53:52Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	fanquake
Stale ACK	maflcko, delta1

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29282 (depends: Ensure definitions are passed when building SQLite with DEBUG=1 by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

delta1 · 2024-01-20T13:51:43Z

concept ACK 4658c83

dergoegge · 2024-01-22T10:45:19Z

Could this be the root cause for #27222?

maflcko · 2024-01-22T16:52:01Z

depends/packages/sqlite.mk

@@ -11,7 +11,8 @@ $(package)_config_opts_linux=--with-pic
 $(package)_config_opts_freebsd=--with-pic
 $(package)_config_opts_netbsd=--with-pic
 $(package)_config_opts_openbsd=--with-pic
-$(package)_config_opts_debug=--enable-debug
+# We avoid using `--enable-debug` because it overrides CFLAGS, a behavior we want to prevent.
+$(package)_cppflags_debug += -DSQLITE_DEBUG -DSQLITE_ENABLE_SELECTTRACE -DSQLITE_ENABLE_WHERETRACE


Why would the latter two be needed? Using only runtime checks/asserts would be more in line with the documentation. See: DEBUG: Disable some optimizations and enable more runtime checking. Also they are not supported for third-party code, see https://sqlite.org/debugging.html

Would be better to only use SQLITE_DEBUG? https://sqlite.org/compile.html#debug

Would be better to only use SQLITE_DEBUG?

Thanks! Done.

maflcko · 2024-01-23T08:28:50Z

lgtm ACK 19fbbb1

delta1 · 2024-01-23T08:30:17Z

ACK 19fbbb1

DrahtBot · 2024-01-23T19:52:55Z

Guix builds (on x86_64)

File	commit `651fb03` (master)	commit `8c26890` (master and this pull)
SHA256SUMS.part	`4154f5bac70c0e80...`	`e0b8c1aa985d218c...`
*-aarch64-linux-gnu-debug.tar.gz	`bb25b4ce47812ea5...`	`63f469e3701dc589...`
*-aarch64-linux-gnu.tar.gz	`986161232f1e15e2...`	`e7563a6d7180e95e...`
*-arm-linux-gnueabihf-debug.tar.gz	`e49f7dacda7a5da5...`	`283ad976780bd88c...`
*-arm-linux-gnueabihf.tar.gz	`730dc6d1e7dc8352...`	`1de94ddadc4724db...`
*-arm64-apple-darwin-unsigned.tar.gz	`9b68ab0ff9669f53...`	`3252a0d3788f4936...`
*-arm64-apple-darwin-unsigned.zip	`296c47bbe521ef4d...`	`201e9a91072b1719...`
*-arm64-apple-darwin.tar.gz	`8f2fa6778612d7ab...`	`1236ea3dfec7ef14...`
*-powerpc64-linux-gnu-debug.tar.gz	`152c8af6f3b40254...`	`07e9154a482cefe7...`
*-powerpc64-linux-gnu.tar.gz	`ad79412dd9f3336a...`	`90ebad757cec9e77...`
*-powerpc64le-linux-gnu-debug.tar.gz	`f4708f3ef7552a96...`	`184f465e465ac3a8...`
*-powerpc64le-linux-gnu.tar.gz	`61d5b2af13263b10...`	`fbd729d963571f51...`
*-riscv64-linux-gnu-debug.tar.gz	`ad4e6e4c7f58acb6...`	`41c803ef23d4b0d1...`
*-riscv64-linux-gnu.tar.gz	`2ac52b8c4d205802...`	`6b5005a94026654f...`
*-x86_64-apple-darwin-unsigned.tar.gz	`2220d4f7a315dd8d...`	`450057a7222e01e9...`
*-x86_64-apple-darwin-unsigned.zip	`e06afdc1aa2fad2a...`	`f7d76e5b79810270...`
*-x86_64-apple-darwin.tar.gz	`8fea74583651139d...`	`feaa39cf94d074fe...`
*-x86_64-linux-gnu-debug.tar.gz	`be7711356c2ed878...`	`8680971c6e977b57...`
*-x86_64-linux-gnu.tar.gz	`12a0dcf7b31e7e41...`	`e7e19785bfcabf9d...`
*.tar.gz	`d6d784acfb57cc72...`	`49aed57a9b76ee8e...`
guix_build.log	`00e936534df3a9b2...`	`7324dae53f56b0f3...`
guix_build.log.diff		`a6e5d939541fd1e6...`

fanquake · 2024-01-24T15:10:17Z

Changes in #29282 might not be strictly required now. However, I consider them an improvement.

I think it'd be easiest to make (any) additional changes together here, so we don't have to go over the flags twice. Can you combine any relevant changes and close the other PR.

Could this be the root cause for #27222?

I think so. Rebased #27495 on top.

Related, I guess -g will no-longer be used, when compiling the lib for debugging?

The SQLite build system overrides the `CFLAGS` when is configured with the `--enable-debug` option.

The `--enable-debug` configure option for the SQLite package does two things. It adds three preprocessor definitions and overrides CFLAGS with "-g -O0". The latter breaks the user's ability to provide sanitizer and LTO flags.

hebasto · 2024-01-25T12:27:47Z

@fanquake Thank you for your review!

Changes in #29282 might not be strictly required now. However, I consider them an improvement.

I think it'd be easiest to make (any) additional changes together here, so we don't have to go over the flags twice. Can you combine any relevant changes and close the other PR.

#29282 has been integrated into this PR.

Related, I guess -g will no-longer be used, when compiling the lib for debugging?

Added $(package)_cflags_debug += -g.

fanquake · 2024-01-25T14:23:13Z

oss-fuzz PR: google/oss-fuzz#11540

fanquake

ACK 5fb8f0f - downstream is also green, so i'll fixup the PR there.

fanquake · 2024-01-25T15:37:14Z

depends/packages/sqlite.mk

-$(package)_cflags+=-DSQLITE_OMIT_SHARED_CACHE -DSQLITE_OMIT_JSON -DSQLITE_LIKE_DOESNT_MATCH_BLOBS
-$(package)_cflags+=-DSQLITE_OMIT_DECLTYPE -DSQLITE_OMIT_PROGRESS_CALLBACK -DSQLITE_OMIT_AUTOINIT
+# We avoid using `--enable-debug` because it overrides CFLAGS, a behavior we want to prevent.
+$(package)_cflags_debug += -g


This should really be done globally, but I can follow up, as I have another global change to make at the same time.

fanquake · 2024-01-25T15:38:07Z

depends/packages/sqlite.mk

-$(package)_cflags+=-DSQLITE_DQS=0 -DSQLITE_DEFAULT_MEMSTATUS=0 -DSQLITE_OMIT_DEPRECATED
-$(package)_cflags+=-DSQLITE_OMIT_SHARED_CACHE -DSQLITE_OMIT_JSON -DSQLITE_LIKE_DOESNT_MATCH_BLOBS
-$(package)_cflags+=-DSQLITE_OMIT_DECLTYPE -DSQLITE_OMIT_PROGRESS_CALLBACK -DSQLITE_OMIT_AUTOINIT
+# We avoid using `--enable-debug` because it overrides CFLAGS, a behavior we want to prevent.


This comment could be a bit confusing, because it says we don't want to use --enable-debug, because it overrides cflags, but then we override cflags in the next line. Not a blocker though.

Should be removed after bitcoin/bitcoin#29287 upstream.

…SQLite with `DEBUG=1` 5fb8f0f depends: Do not override CFLAGS when building SQLite with DEBUG=1 (Hennadii Stepanov) 2b0dd88 depends: Ensure definitions are passed when building SQLite with DEBUG=1 (Hennadii Stepanov) Pull request description: The `--enable-debug` configure option for the SQLite package does two things: ```autoconf #----------------------------------------------------------------------- # --enable-debug # AC_ARG_ENABLE(debug, [AS_HELP_STRING( [--enable-debug], [build with debugging features enabled [default=no]])], [], []) AC_MSG_CHECKING([Build type]) if test x"$enable_debug" = "xyes"; then BUILD_CFLAGS="$BUILD_CFLAGS -DSQLITE_DEBUG -DSQLITE_ENABLE_SELECTTRACE -DSQLITE_ENABLE_WHERETRACE" CFLAGS="-g -O0" AC_MSG_RESULT([debug]) else AC_MSG_RESULT([release]) fi #----------------------------------------------------------------------- ``` It adds three preprocessor definitions and overrides `CFLAGS` with `"-g -O0"`. The latter breaks the user's ability to provide sanitizer and LTO flags. This PR might be especially useful for OSS-Fuzz where `DEBUG=1` has been used since google/oss-fuzz#10503. Also it makes a workaround for building SQLite for 32-bit unneeded. For details, please refer to https://github.com/hebasto/oss-fuzz/tree/240120-sqlite. Changes in bitcoin#29282 might not be strictly required now. However, I consider them an improvement. ACKs for top commit: fanquake: ACK 5fb8f0f - downstream is also green, so i'll fixup the PR there. Tree-SHA512: 8593d8a0237ebb270d5da763fb65ed642ab8ed0d44e57704a34154621f49e3d5c58b462cc0070251fa1ba556c58a3c7d3620530d6839dc6dc9e0887010330eca

801c4fc build: followup to 29488 applied to gmp (pasta) 4b704a6 Merge bitcoin#28627: depends: zeromq 4.3.5 (fanquake) 0e6cb98 Merge bitcoin#26421: build: copy config.{guess,sub} post autogen in zmq package (fanquake) cd33b69 Merge bitcoin#29488: depends: always configure with `--with-pic` (fanquake) 1b88674 Merge bitcoin#29287: depends: Do not override `CFLAGS` when building SQLite with `DEBUG=1` (fanquake) f50fb6e Merge bitcoin#26998: depends: ensure we are appending to sqlite cflags (Andrew Chow) 25a594f Merge bitcoin#25987: build: compile depends sqlite with more recommended options (Andrew Chow) d725c58 Merge bitcoin#27312: depends: qrencode 4.1.1 (fanquake) 482e5bb Merge bitcoin#27462: depends: fix compiling bdb with clang-16 on aarch64 (fanquake) f0a53c9 Merge bitcoin#26994: depends: define `__BSD_VISIBLE` for FreeBSD bdb build (merge-script) b1ac992 Merge bitcoin#26073: build: fix depends bdb compilation for BSDs (fanquake) 45e0f6e Merge bitcoin#25763: bdb: disable Werror for format-security (fanquake) Pull request description: ## Issue being fixed or feature implemented First batch of PRs which lead towards cmake, focusing on having nothing or very little done out of order / with significant conflicts ## What was done? batch of 11 backports which lead towards cmake ## How Has This Been Tested? Built locally, seems to work, let's see CI ## Breaking Changes ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 801c4fc knst: utACK 801c4fc Tree-SHA512: b8b5299da9d82ab485ba5141ea12ba5c606f1a783b34c957d61e0a68d45865754fbc8bcbb0c5eabe3d410ff6262ce26789cf4a3af696905f7b7908e523c97816

…SQLite with `DEBUG=1` 5fb8f0f depends: Do not override CFLAGS when building SQLite with DEBUG=1 (Hennadii Stepanov) 2b0dd88 depends: Ensure definitions are passed when building SQLite with DEBUG=1 (Hennadii Stepanov) Pull request description: The `--enable-debug` configure option for the SQLite package does two things: ```autoconf #----------------------------------------------------------------------- # --enable-debug # AC_ARG_ENABLE(debug, [AS_HELP_STRING( [--enable-debug], [build with debugging features enabled [default=no]])], [], []) AC_MSG_CHECKING([Build type]) if test x"$enable_debug" = "xyes"; then BUILD_CFLAGS="$BUILD_CFLAGS -DSQLITE_DEBUG -DSQLITE_ENABLE_SELECTTRACE -DSQLITE_ENABLE_WHERETRACE" CFLAGS="-g -O0" AC_MSG_RESULT([debug]) else AC_MSG_RESULT([release]) fi #----------------------------------------------------------------------- ``` It adds three preprocessor definitions and overrides `CFLAGS` with `"-g -O0"`. The latter breaks the user's ability to provide sanitizer and LTO flags. This PR might be especially useful for OSS-Fuzz where `DEBUG=1` has been used since google/oss-fuzz#10503. Also it makes a workaround for building SQLite for 32-bit unneeded. For details, please refer to https://github.com/hebasto/oss-fuzz/tree/240120-sqlite. Changes in bitcoin#29282 might not be strictly required now. However, I consider them an improvement. ACKs for top commit: fanquake: ACK 5fb8f0f - downstream is also green, so i'll fixup the PR there. Tree-SHA512: 8593d8a0237ebb270d5da763fb65ed642ab8ed0d44e57704a34154621f49e3d5c58b462cc0070251fa1ba556c58a3c7d3620530d6839dc6dc9e0887010330eca

hebasto added the Build system label Jan 20, 2024

DrahtBot mentioned this pull request Jan 20, 2024

depends: Ensure definitions are passed when building SQLite with DEBUG=1 #29282

Closed

maflcko reviewed Jan 22, 2024

View reviewed changes

hebasto force-pushed the 240120-sqlite branch from 4658c83 to 19fbbb1 Compare January 22, 2024 17:01

maflcko added the DrahtBot Guix build requested label Jan 23, 2024

DrahtBot removed the DrahtBot Guix build requested label Jan 23, 2024

maflcko requested a review from fanquake January 24, 2024 08:14

maflcko mentioned this pull request Jan 25, 2024

test: use-of-uninitialized-value in sqlite3Strlen30 #27222

Closed

hebasto added 2 commits January 25, 2024 12:23

depends: Ensure definitions are passed when building SQLite with DEBUG=1

2b0dd88

The SQLite build system overrides the `CFLAGS` when is configured with the `--enable-debug` option.

hebasto force-pushed the 240120-sqlite branch from 19fbbb1 to 5fb8f0f Compare January 25, 2024 12:25

fanquake mentioned this pull request Jan 25, 2024

bitcoin-core: drop 32bit sqlite workaround google/oss-fuzz#11540

Merged

fanquake approved these changes Jan 25, 2024

View reviewed changes

DrahtBot requested a review from maflcko January 25, 2024 15:41

fanquake merged commit ac923e7 into bitcoin:master Jan 25, 2024

fanquake added a commit to fanquake/oss-fuzz that referenced this pull request Jan 25, 2024

bitcoin-core: drop 32bit sqlite workaround

c8576c3

Should be removed after bitcoin/bitcoin#29287 upstream.

hebasto deleted the 240120-sqlite branch January 25, 2024 15:48

DavidKorczynski pushed a commit to google/oss-fuzz that referenced this pull request Jan 25, 2024

bitcoin-core: drop 32bit sqlite workaround (#11540)

b2225e3

Should be removed after bitcoin/bitcoin#29287 upstream.

bitcoin locked and limited conversation to collaborators Jan 24, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

depends: Do not override `CFLAGS` when building SQLite with `DEBUG=1` #29287

depends: Do not override `CFLAGS` when building SQLite with `DEBUG=1` #29287

Uh oh!

hebasto commented Jan 20, 2024

Uh oh!

DrahtBot commented Jan 20, 2024 •

edited

Loading

Uh oh!

delta1 commented Jan 20, 2024

Uh oh!

dergoegge commented Jan 22, 2024

Uh oh!

maflcko Jan 22, 2024

Uh oh!

hebasto Jan 22, 2024

Uh oh!

maflcko commented Jan 23, 2024

Uh oh!

delta1 commented Jan 23, 2024

Uh oh!

DrahtBot commented Jan 23, 2024

Uh oh!

fanquake commented Jan 24, 2024

Uh oh!

hebasto commented Jan 25, 2024

Uh oh!

fanquake commented Jan 25, 2024

Uh oh!

fanquake left a comment

Uh oh!

fanquake Jan 25, 2024

Uh oh!

fanquake Jan 25, 2024

Uh oh!

Uh oh!

depends: Do not override CFLAGS when building SQLite with DEBUG=1 #29287

depends: Do not override CFLAGS when building SQLite with DEBUG=1 #29287

Uh oh!

Conversation

hebasto commented Jan 20, 2024

Uh oh!

DrahtBot commented Jan 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

delta1 commented Jan 20, 2024

Uh oh!

dergoegge commented Jan 22, 2024

Uh oh!

maflcko Jan 22, 2024

Choose a reason for hiding this comment

Uh oh!

hebasto Jan 22, 2024

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jan 23, 2024

Uh oh!

delta1 commented Jan 23, 2024

Uh oh!

DrahtBot commented Jan 23, 2024

Guix builds (on x86_64)

Uh oh!

fanquake commented Jan 24, 2024

Uh oh!

hebasto commented Jan 25, 2024

Uh oh!

fanquake commented Jan 25, 2024

Uh oh!

fanquake left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake Jan 25, 2024

Choose a reason for hiding this comment

Uh oh!

fanquake Jan 25, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

depends: Do not override `CFLAGS` when building SQLite with `DEBUG=1` #29287

depends: Do not override `CFLAGS` when building SQLite with `DEBUG=1` #29287

DrahtBot commented Jan 20, 2024 •

edited

Loading