On 23/01/2026 08:32, Richard Biener wrote:
> On Thu, 22 Jan 2026, Alex Coplan wrote:
>
> > This patch makes the constructor of bbitmap explicit and deals with the
> > fallout throughout the codebase.
> >
> > The goal of this is to avoid the re-occurrence of bugs such as PR123206.
> > In particular, it prevents implicit conversions from other types
> > (integers, booleans, pointers (!), etc.) in bbitmap context; instead one
> > must directly construct a bbitmap to use it in such a context.
> >
> > Doing this revealed a bug in bbitmap: the destructive bitwise operators
> > (|=, &=, ^=) all returned this (a pointer) which was being implicitly
> > converted to a bbitmap. To verify this, I added some selftests for
> > these operators which indeed failed. Of course this latent bug becomes
> > a compile error when the ctor is marked explicit, which is much nicer.
> > This patch fixes that bug.
> >
> > To see another benefit of this change, consider PR123206 on AArch64. The
> > previous patch marks the aarch64_pragma_builtins table as constexpr,
> > which gives errors such as this in the presence of the bug:
> >
> > aarch64-builtins.cc:1736:1: error: the value of ‘global_options’ is not
> > usable in a constant expression
> >
> > but with this change, we get clearer error messages which give more of
> > the context:
> >
> > $GCC/gcc/config/aarch64/aarch64.h:242:39: error: cannot convert ‘bool’ to
> > ‘aarch64_feature_flags’ {aka ‘bbitmap<2>’}
> > 242 | #define TARGET_SIMD (TARGET_BASE_SIMD && TARGET_NON_STREAMING)
> > | ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~
> > | |
> > | bool
> > $GCC/gcc/config/aarch64/aarch64-simd-pragma-builtins.def:200:48: note: in
> > expansion of macro ‘TARGET_SIMD’
> > 200 | #define REQUIRED_EXTENSIONS nonstreaming_only (TARGET_SIMD)
> > | ^~~~~~~~~~~
> > $GCC/gcc/config/aarch64/aarch64-builtins.cc:1720:33: note: in expansion of
> > macro ‘REQUIRED_EXTENSIONS’
> > 1720 | aarch64_required_extensions::REQUIRED_EXTENSIONS, FLAG_##F},
> > | ^~~~~~~~~~~~~~~~~~~
> > $GCC/gcc/config/aarch64/aarch64-simd-pragma-builtins.def:51:3: note: in
> > expansion of macro ‘ENTRY’
> > 51 | ENTRY (N, ternary, T0, T1, T2, T3, U, F)
> > | ^~~~~
> > $GCC/gcc/config/aarch64/aarch64-simd-pragma-builtins.def:201:1: note: in
> > expansion of macro ‘ENTRY_TERNARY’
> > 201 | ENTRY_TERNARY (vbsl_mf8, mf8, u8, mf8, mf8, UNSPEC_BSL, QUIET)
> > | ^~~~~~~~~~~~~
> >
> > The main downside of this patch is that it makes usage of
> > aarch64_feature_flags slightly less ergonomic; contexts that previously
> > passed an integer constant 0 now need an object of type
> > aarch64_feature_flags. I've defined a macro AARCH64_FL_NONE to slightly
> > reduce the amount of typing needed (compared to aarch64_feature_flags (0)),
> > but it does make some of the code a little more verbose.
> >
> > IMO the trade-off is worth it to avoid further mishaps such as the PR in
> > question (and the buggy bbitmap operators), but I would like to hear
> > others' thoughts on it, too.
> >
> > Bootstrapped/regtested on aarch64-linux-gnu, OK for trunk?
>
> The middle-end parts look OK. I supose it would be nice to split
> them out in case backporting is required separately from the aarch64
> changes (but I didn't know about bbitmap, so maybe it's only used
> by aarch64).
Thanks. It looks to be also used in emit-rtl.cc:rtx_expander, as of
r16-788-gd63c889d5cd3ef.
I think we can probably get away without backporting the middle-end
changes in this case, as I think nothing currently depends on the return
value of the destructive bbitmap operators, which is the only
correctness issue addressed by this patch.
Alex
>
> Richard.
>
> > I'm happy to split this up or defer completely until next stage 1, if
> > folks prefer.
> >
> > Thanks,
> > Alex
> >
> > gcc/ChangeLog:
> >
> > PR target/123206
> > * Makefile.in (OBJS): Add new bbitmap-selftest.o.
> > * bbitmap.h (bbitmap): Make ctor explict, fix destructive operators to
> > dereference 'this' on return.
> > * common/config/aarch64/aarch64-common.cc (all_extensions): Replace
> > uses of
> > literal 0 with AARCH64_FL_NONE.
> > (all_architectures): Likewise.
> > (all_cores): Likewise.
> > (aarch64_get_extension_string_for_isa_flags): Likewise, and also switch
> > from
> > using assignment to braced initialization for aarch64_feature_flags.
> > * config/aarch64/aarch64-builtins.cc
> > (aarch64_check_required_extensions):
> > Likewise.
> > (aarch64_general_required_extensions): Likewise, and avoid comparing
> > against literal 0.
> > * config/aarch64/aarch64-feature-deps.h (get_flags): Use AARCH64_FL_NONE
> > instead of literal 0.
> > (get_enable): Likewise.
> > (alias_prefer_over_flags_##IDENT): Likewise.
> > * config/aarch64/aarch64-protos.h (struct aarch64_target_switcher):
> > Likewise.
> > (struct aarch64_required_extensions): Likewise.
> > * config/aarch64/aarch64-simd-pragma-builtins.def: Likewise.
> > * config/aarch64/aarch64-sve-builtins-base.def: Likewise.
> > * config/aarch64/aarch64-sve-builtins-sme.def: Likewise.
> > * config/aarch64/aarch64-sve-builtins-sve2.def: Likewise.
> > * config/aarch64/aarch64-sve-builtins.cc (DEF_NEON_SVE_FUNCTION):
> > Likewise.
> > (function_builder::get_attributes): Don't compare aarch64_feature_flags
> > values
> > against literal 0.
> > * config/aarch64/aarch64-sve-builtins.h
> > (function_expander::get_contiguous_base): Replace 0 with
> > AARCH64_FL_NONE.
> > * config/aarch64/aarch64.cc (all_cores): Likewise.
> > (aarch64_override_options): Likewise, plus use of braced initialization
> > instead of assignment.
> > (aarch64_process_target_attr): Use braced initialization for
> > isa_temp.
> > (aarch64_valid_sysreg_name_p): Avoid comparison of
> > aarch64_feature_flags against literal 0.
> > (aarch64_retrieve_sysreg): Likewise.
> > * config/aarch64/aarch64.h (AARCH64_FL_NONE): New.
> > (TARGET_STREAMING_COMPATIBLE): Avoid comparison of
> > aarch64_feature_flags value against literal 0.
> > * config/aarch64/driver-aarch64.cc (aarch64_cpu_data): Replace
> > use of 0 with AARCH64_FL_NONE.
> > (aarch64_arches): Likewise.
> > (host_detect_local_cpu): Use braced initialization for
> > aarch64_feature_flags.
> > * selftest-run-tests.cc (selftest::run_tests): Add bbitmap_cc_tests.
> > * selftest.h (bbitmap_cc_tests): New.
> > * bbitmap-selftests.cc: New file.
> > ---
> > gcc/Makefile.in | 1 +
> > gcc/bbitmap-selftests.cc | 69 +++++++++++++++++++
> > gcc/bbitmap.h | 8 +--
> > gcc/common/config/aarch64/aarch64-common.cc | 16 +++--
> > gcc/config/aarch64/aarch64-builtins.cc | 12 ++--
> > gcc/config/aarch64/aarch64-feature-deps.h | 7 +-
> > gcc/config/aarch64/aarch64-protos.h | 10 +--
> > .../aarch64/aarch64-simd-pragma-builtins.def | 2 +-
> > .../aarch64/aarch64-sve-builtins-base.def | 4 +-
> > .../aarch64/aarch64-sve-builtins-sme.def | 4 +-
> > .../aarch64/aarch64-sve-builtins-sve2.def | 4 +-
> > gcc/config/aarch64/aarch64-sve-builtins.cc | 8 +--
> > gcc/config/aarch64/aarch64-sve-builtins.h | 2 +-
> > gcc/config/aarch64/aarch64.cc | 18 ++---
> > gcc/config/aarch64/aarch64.h | 4 +-
> > gcc/config/aarch64/driver-aarch64.cc | 10 +--
> > gcc/selftest-run-tests.cc | 1 +
> > gcc/selftest.h | 1 +
> > 18 files changed, 129 insertions(+), 52 deletions(-)
> > create mode 100644 gcc/bbitmap-selftests.cc
> >
> >
>
> --
> Richard Biener <[email protected]>
> SUSE Software Solutions Germany GmbH,
> Frankenstrasse 146, 90461 Nuernberg, Germany;
> GF: Jochen Jaser, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
