Skip to content

Conversation

@jhale
Copy link
Member

@jhale jhale commented Nov 20, 2025

This PR:

  • fixes the unsigned int bug discovered by @drew-parsons related to generating a uniform distribution using negative lower bound, despite the integers type being unsigned.
  • Ensures that our Developer mode triggers exceptions in the C++ standard library which allowed Drew to discover the bug. I suspect this was accidentally turned off by using -O2 but not completely certain.

As an aside, there are quite a number of useful compile warnings not included -Wall which we should consider turning on.

These extra checks produce no discernable increase in test time.

@jhale jhale changed the title Fix unsigned int bug in sort test and enable more debugging Fix unsigned int bug in sort test and C++ standard library assertions Nov 20, 2025
@jhale jhale marked this pull request as ready for review November 20, 2025 12:03
@jhale jhale added this pull request to the merge queue Nov 20, 2025
Merged via the queue into main with commit f6db72f Nov 20, 2025
19 checks passed
@jhale jhale deleted the jhale/fix-unsigned-int-bug-test branch November 20, 2025 12:29
@qbisi
Copy link

qbisi commented Nov 22, 2025

I'm curious about how the failing test was identified. In Nixpkgs, we run the DOLFINx C++ unit tests, and they appear to pass.

logs: https://logs.ofborg.org/?key=nixos/nixpkgs.460685&attempt_id=72429051-7804-43da-af4b-3ba47e85a033

@jhale
Copy link
Member Author

jhale commented Nov 22, 2025

My understanding is that Debian and Ubuntu are starting to use GCC options like -fhardening which enables hardened C++ standard libraries with more asserts enabled, amongst other things. This picked up this bug, but we didn't see it in our CI.

To avoid this, we will be enabling more hardening on our CI systems, which use our custom CMake Developer build type, over the coming weeks.

We do not plan to adjust the standard CMake built types, e.g. Release - we leave this to downstream packagers discretion.

@drew-parsons
Copy link
Contributor

drew-parsons commented Nov 22, 2025

Debian also uses gcc-15 now, which has tightened up standards (C23 language standard by default) and catches use of old conventions for function definitions.

The flags debian uses are managed via dpkg-buildflags

$ dpkg-buildflags
ASFLAGS=
ASFLAGS_FOR_BUILD=
CFLAGS=-g -O2 -Werror=implicit-function-declaration -ffile-prefix-map=/projects/fenics/build/fenics-dolfinx=. -fstack-protector-strong -fstack-clash-protection -Wformat -Werror=format-security -fcf-protection
CFLAGS_FOR_BUILD=-g -O2 -Werror=implicit-function-declaration -ffile-prefix-map=/projects/fenics/build/fenics-dolfinx=. -fstack-protector-strong -fstack-clash-protection -Wformat -Werror=format-security -fcf-protection
CPPFLAGS=-Wdate-time -D_FORTIFY_SOURCE=2
CPPFLAGS_FOR_BUILD=-Wdate-time -D_FORTIFY_SOURCE=2
CXXFLAGS=-g -O2 -ffile-prefix-map=/projects/fenics/build/fenics-dolfinx=. -fstack-protector-strong -fstack-clash-protection -Wformat -Werror=format-security -fcf-protection
CXXFLAGS_FOR_BUILD=-g -O2 -ffile-prefix-map=/projects/fenics/build/fenics-dolfinx=. -fstack-protector-strong -fstack-clash-protection -Wformat -Werror=format-security -fcf-protection
DFLAGS=-frelease
DFLAGS_FOR_BUILD=-frelease
FCFLAGS=-g -O2 -ffile-prefix-map=/projects/fenics/build/fenics-dolfinx=. -fstack-protector-strong -fstack-clash-protection -fcf-protection
FCFLAGS_FOR_BUILD=-g -O2 -ffile-prefix-map=/projects/fenics/build/fenics-dolfinx=. -fstack-protector-strong -fstack-clash-protection -fcf-protection
FFLAGS=-g -O2 -ffile-prefix-map=/projects/fenics/build/fenics-dolfinx=. -fstack-protector-strong -fstack-clash-protection -fcf-protection
FFLAGS_FOR_BUILD=-g -O2 -ffile-prefix-map=/projects/fenics/build/fenics-dolfinx=. -fstack-protector-strong -fstack-clash-protection -fcf-protection
LDFLAGS=-Wl,-z,relro
LDFLAGS_FOR_BUILD=-Wl,-z,relro
OBJCFLAGS=-g -O2 -ffile-prefix-map=/projects/fenics/build/fenics-dolfinx=. -fstack-protector-strong -fstack-clash-protection -Wformat -Werror=format-security -fcf-protection
OBJCFLAGS_FOR_BUILD=-g -O2 -ffile-prefix-map=/projects/fenics/build/fenics-dolfinx=. -fstack-protector-strong -fstack-clash-protection -Wformat -Werror=format-security -fcf-protection
OBJCXXFLAGS=-g -O2 -ffile-prefix-map=/projects/fenics/build/fenics-dolfinx=. -fstack-protector-strong -fstack-clash-protection -Wformat -Werror=format-security -fcf-protection
OBJCXXFLAGS_FOR_BUILD=-g -O2 -ffile-prefix-map=/projects/fenics/build/fenics-dolfinx=. -fstack-protector-strong -fstack-clash-protection -Wformat -Werror=format-security -fcf-protection

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants