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

hebasto commented at 11:53 AM on January 20, 2024: member

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 https://github.com/google/oss-fuzz/pull/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.

hebasto added the label Build system on Jan 20, 2024

DrahtBot commented at 11:53 AM on January 20, 2024: contributor

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 commented at 1:51 PM on January 20, 2024: none

concept ACK 4658c83

dergoegge commented at 10:45 AM on January 22, 2024: member

Could this be the root cause for #27222?

in depends/packages/sqlite.mk:15 in 4658c8375e outdated

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

maflcko commented at 4:52 PM on January 22, 2024:

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

hebasto commented at 5:01 PM on January 22, 2024:

Would be better to only use SQLITE_DEBUG?

Thanks! Done.

hebasto force-pushed on Jan 22, 2024

maflcko commented at 8:28 AM on January 23, 2024: member

lgtm ACK 19fbbb1d00f40095b0b6da5941ff069376800e7f

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

delta1 commented at 8:30 AM on January 23, 2024: none

ACK 19fbbb1

DrahtBot commented at 7:52 PM on January 23, 2024: contributor

Guix builds (on x86_64)

File	commit 651fb034d85eb5db561bfd24b74f7271417defa5<br>(master)	commit 8c268903548c9e8ca9825173481b010829adc138<br>(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...`

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

maflcko requested review from fanquake on Jan 24, 2024

fanquake commented at 3:10 PM on January 24, 2024: member

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?

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

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

2b0dd88f1c

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

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.

5fb8f0f80f

hebasto force-pushed on Jan 25, 2024

hebasto commented at 12:27 PM on January 25, 2024: member

@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 referenced this in commit a59ae6b5e8 on Jan 25, 2024

fanquake referenced this in commit 16580aee3e on Jan 25, 2024

fanquake referenced this in commit 0d6fa8217d on Jan 25, 2024

fanquake commented at 2:23 PM on January 25, 2024: member

oss-fuzz PR: https://github.com/google/oss-fuzz/pull/11540

in depends/packages/sqlite.mk:15 in 5fb8f0f80f

  10 | @@ -11,10 +11,12 @@ $(package)_config_opts_linux=--with-pic
  11 |  $(package)_config_opts_freebsd=--with-pic
  12 |  $(package)_config_opts_netbsd=--with-pic
  13 |  $(package)_config_opts_openbsd=--with-pic
  14 | -$(package)_config_opts_debug=--enable-debug
  15 | -$(package)_cflags+=-DSQLITE_DQS=0 -DSQLITE_DEFAULT_MEMSTATUS=0 -DSQLITE_OMIT_DEPRECATED
  16 | -$(package)_cflags+=-DSQLITE_OMIT_SHARED_CACHE -DSQLITE_OMIT_JSON -DSQLITE_LIKE_DOESNT_MATCH_BLOBS
  17 | -$(package)_cflags+=-DSQLITE_OMIT_DECLTYPE -DSQLITE_OMIT_PROGRESS_CALLBACK -DSQLITE_OMIT_AUTOINIT
  18 | +# We avoid using `--enable-debug` because it overrides CFLAGS, a behavior we want to prevent.
  19 | +$(package)_cflags_debug += -g

fanquake commented at 3:37 PM on January 25, 2024:

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

in depends/packages/sqlite.mk:14 in 5fb8f0f80f

  10 | @@ -11,10 +11,12 @@ $(package)_config_opts_linux=--with-pic
  11 |  $(package)_config_opts_freebsd=--with-pic
  12 |  $(package)_config_opts_netbsd=--with-pic
  13 |  $(package)_config_opts_openbsd=--with-pic
  14 | -$(package)_config_opts_debug=--enable-debug
  15 | -$(package)_cflags+=-DSQLITE_DQS=0 -DSQLITE_DEFAULT_MEMSTATUS=0 -DSQLITE_OMIT_DEPRECATED
  16 | -$(package)_cflags+=-DSQLITE_OMIT_SHARED_CACHE -DSQLITE_OMIT_JSON -DSQLITE_LIKE_DOESNT_MATCH_BLOBS
  17 | -$(package)_cflags+=-DSQLITE_OMIT_DECLTYPE -DSQLITE_OMIT_PROGRESS_CALLBACK -DSQLITE_OMIT_AUTOINIT
  18 | +# We avoid using `--enable-debug` because it overrides CFLAGS, a behavior we want to prevent.

fanquake commented at 3:38 PM on January 25, 2024:

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.

fanquake approved

fanquake commented at 3:41 PM on January 25, 2024: member

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

DrahtBot requested review from maflcko on Jan 25, 2024

fanquake merged this on Jan 25, 2024

fanquake closed this on Jan 25, 2024

fanquake referenced this in commit c8576c349e on Jan 25, 2024

hebasto deleted the branch on Jan 25, 2024

DavidKorczynski referenced this in commit b2225e3e63 on Jan 25, 2024

knst referenced this in commit 632ffb8388 on Aug 23, 2024

PastaPastaPasta referenced this in commit 1b88674815 on Sep 27, 2024

PastaPastaPasta referenced this in commit 3b9f9352a1 on Sep 27, 2024

PastaPastaPasta referenced this in commit 1d02c15258 on Sep 27, 2024

bitcoin locked this on Jan 24, 2025