Re: [PATCH gcc/* 2/2] gcc: stop using 'int' to represent sets of qualifiers

On 7/5/26 5:15 PM, Arsen Arsenović wrote:
> The set of all qualifiers present on a type consists of the const,
> volatile, restrict and atomic qualification, which may be either present
> or absent, and the address space qualifier, which may be one of many
> values (one of which is the "generic" address space, present on all
> platforms, used in absence of another address space; it is also the
> address space qualifier on which all standard library routines operate).
> 
> For the former four, 'int' serves us okay; using ints as sets is a
> well-understood pattern (but it leads to problems like those described
> here when those ints cease to be sets).  Since they're either present or
> absent, treating an int as a bit-set is convenient and simple.  But,
> with the introduction of address space qualifiers into the mix, the
> semantics of these operators becomes incorrect.
> 
> Take, for instance, the following line of code:
> 
>    quals_union = quals1 | quals2;
> 
> ... (where quals1, quals2 are TYPE_QUALS of some types)
> 
> Only in cases where DECODE_QUAL_ADDR_SPACE (quals1) ==
> DECODE_QUAL_ADDR_SPACE (quals2), or where one of those address spaces is
> generic, and the other a super/subset of the generic address space, does
> this yields what the author intended.  In all other cases, the operation
> above yields subtly incorrect results, while the compiler is (naturally)
> silent about it.
> 
> This issue is not theoretical either.  In the implementation of the C++
> Named Address Spaces support currently being worked on and discussed on
> the mailing list[2], the following broken testcase arose (for the GCN
> target; __flat address space is AS1, __lds is AS2, and __gds is AS3;
> __flat is a superset address space to __lds):
> 
>    template<typename T>
>    __flat T *
>    foo ();
> 
>    void
>    bar ()
>    { **foo<__lds int> (); }
> 
> The above testcase produces the diagnostic:
> 
>    <stdin>:1:52: error: invalid type argument of unary '*' (have '__gds int')
> 
> It is obvious how this came about: the frontend used '|' to merge two
> sets of qualifiers, presuming that such a merge can never fail, because
> the union of two sets is a valid and complete function, and that the
> bit-OR of these two bitsets is the union of the corresponding two sets
> of qualifiers.
> 
> However, this is not so: with the introduction of address spaces, the
> union of two sets of qualifiers ceases to coincide with the bit-OR
> operator, and the union becomes a partial function.
> 
> To demonstrate the former, we can use the testcase above.  In it, the
> C++ frontend was trying to take the union of the qualifier sets {__lds}
> and {__flat}.  These were previously represented as 0x0100 and 0x0200.
> Ergo, the result of the bit-op was 0x0300.
> 
> 0x0300 corresponds to the qualifier set {__gds}.  This is how we got the
> bad diagnostic above.  This is an instance of the "bit-OR no longer
> coincides with set union (except by accident)" problem; the union of
> those two sets (in most cases, anyway) is {__flat}.
> 
> But, even if we were to fix this initial problem, we still have the
> problem of the union of qualifier sets becoming a partial function.
> 
> This one can be demonstrated in C, as the C frontend already deals with
> this problem in an ad-hoc fashion.
> 
> In the following case, the __lds and __global address spaces are
> distinct (though they are both subsets of the __flat address space):
> 
>    typedef __lds int lds_int;
>    void foo (__global lds_int *x);
> 
> ... the C frontend issues the diagnostic:
> 
>    <stdin>:2:31: error: conflicting named address spaces (__global vs __lds)
> 
> It had to do so because the union of the qualifier sets {__lds} and
> {__global} for the purposes of this declaration does not exist.[1]
> 
> The way the C FE handles this is ad-hoc: in 'grokdeclarator', it has a
> specific check that covers this case:
> 
>    if (!ADDR_SPACE_GENERIC_P (as1) && !ADDR_SPACE_GENERIC_P (as2) && as1 != as2)
>      error_at (loc, "conflicting named address spaces (%s vs %s)",
> 	      c_addr_space_name (as1), c_addr_space_name (as2));
> 
> Thus, it is quite easy for developers to forget this check.
> 
> What's worse, there is a lot of existing code that presumes that the set
> of qualifiers is actually the set of the four "simple" present/absent
> qualifier (CVRA).  If that presumption changes, there's no way to
> diagnose all sites where this presumption is now broken (as it is in the
> C++ NAS support patch).
> 
> The C++ type system is completely capable of encoding the restrictions
> above, and ergo diagnosing misuse.  Making use of that is the goal of
> this patch.
> 
> First, we make a distinction between cv_qualifier and qualifier_set, as
> many frontends do not care about anything but the cv-qualifiers, and
> because the former have operations not applicable to the latter.
> 
> The former models the CVRA qualifiers, which are either present or
> absent.
> 
> In this revision, I left it as an unscoped enum, but fixed its
> underlying type as 'unsigned char' (so that its size is known to be less
> than that of qualifier_set).  It may be desirable to make it an enum
> class, to forbid the usage of operators like + on it.  This is a lower
> priority since there's no existing code that does so.
> 
> The latter (qualifier_set) models the set of all qualifiers, i.e. CVRA +
> the address space qualifier (at the moment).
> 
> The interesting bits of the patch are in tree-core.h and tree.h.  These
> two provide the new types and matching helper functions.  It may be
> worth breaking them out into bits-style headers, though.  I've tried to
> curb the growth of those headers too much, but the constexpr operators
> and functions were actually needed a few times.
> 
> For cv_qualifiers, I've provided binary bitwise operations, in order to
> inhibit integer promotion.  This makes it so that manual casting isn't
> necessary when using cv_qualifier values.
> 
> For qualifier_set, tree.h lost operations that were made redundant/wrong
> by the change.  In their place, I've provided functions for modifying
> and reading qualifier_set values.
> 
> Note, however, the decision to drop operator& for qualifier_sets.  As it
> turns out, many places in the codebase used patterns like 'q & ~p' to
> remove qualifier P from set Q, but this became incorrect, as it loses
> the address space qualifier also.
> 
> This operation was also often used for simple presence checks, by simply
> checking 'q & p', so I initially made operator& return 'bool', but this
> turned out to silently change the meaning of some existing code, where
> a pattern like 'int cqual = q & TYPE_QUALS_CONST' appeared.
> 
> Hence, I decided it is better not to provide this operator as it opens
> the possibility for easy misuse, and because 'without', 'intercept' and
> 'has' are quite short anyway.
> 
> Qualifier sets may be decomposed into (currently) a pair, that may be
> destructured via std::tie.  This was provided as such to allow inducing
> errors should a new component ever appear on qualifiers sets, even
> though this is quite an unlikely eventuality.  In essence, should
> qualifier sets grow to contain one more member, all the places that do:
> 
>    std::tie (cvquals, addrspace) = quals.split ();
> 
> ... would yell, letting us know what to fix.
> 
> The qualifier set type is 16 bits, and trivially copyable and
> destructible, and so, fits into registers on most machines.  Most of the
> operations on the qualifier set type are also provided as constexpr
> functions, and so, should be very easy for the compiler to optimize
> away.
> 
> The two union operations provided for qualifier sets now are merge and
> join.  These differ in that the former is apt for finding qualification
> that can be used in common for two objects, and that the latter can be
> used to add qualification to an existing qualifier set
> "syntactically" (i.e. as if the keywords were just added to the original
> source code from which the qualifier set was constructed).  These two
> operations were most common in the C++ frontend, especially the latter.
> 
> This version of the patch does not extensively refactor the C frontend
> to utilize the new operations; since it is a blocker for the C++ Named
> Address Spaces support, I didn't prioritize that.
> 
> Reg-strapped on x86_64-linux-gnu, powerpc64le-linux-gnu, and
> (currently being) tested on amdgcn-amdhsa and s390x-ibm-linux-gnu.
> Build-tested for rl78-elf.
> 
> No functional changes intended.

Thanks, and thanks for the pings.

There are several git gcc-style issues that I won't repeat here, most of 
which seem like true positives.

> 	(qualifier_set::qualifier_set): New.  Constructs a qualifier set
> 	from a TREE_NODE.  Used where new expansion of TYPE_QUALS (which
> 	contains a comma) breaks other macros.  :-(

git gcc-verify rejects the emoticon as an unmatched open paren.

But couldn't you avoid this problem by using qualifier_set (...) in 
TYPE_QUALS instead of {...}?

> -	  && TYPE_QUALS (TREE_TYPE (selector_type)) != TYPE_UNQUALIFIED)
> +	  && TYPE_QUALS (TREE_TYPE (selector_type)) != qualifier_set {})

Are all the changes like this necessary?  Don't these two have the same 
result?

> -      && TYPE_QUALS (element_type))
> +      && TYPE_QUALS (element_type) != qualifier_set {})

And how about an explicit operator bool so we don't need these changes?

> -    unqual_elt = c_build_qualified_type (elt, KEEP_QUAL_ADDR_SPACE (quals));
> +    unqual_elt = c_build_qualified_type (elt,
> +					 quals.without (TYPE_QUAL_ALL));
The quals.without (TYPE_QUAL_ALL) pattern seems awkward, especially 
given the semi-ambiguity of "ALL".  How about a without_cv member function?

> +  /* Construct an empty qualifier set with the generic address space.  Such a
> +     qualifier set corresponds to unqualified types.  */
> +  qualifier_set () = default;
> +  static_assert (cv_qualifier {} == TYPE_UNQUALIFIED
> +		 && addr_space_t {} == ADDR_SPACE_GENERIC,
> +		 "We want the trivial default constructor to use those vals");

Is there a reason we need the default constructor to be trivial rather 
than explicitly setting those values?  If so that should be documented 
in the comment.

> +  /* Returns true if qualifiers in SUBSET can be replaced with qualifiers in
> +     THIS safely.
> +
> +     In general, this means that an object qualified per SUBSET can be used as
> +     if it was qualified per this qualifier set (e.g. 'T' as 'const T', or
> +     'const T' as 'const volatile AS1 T', presuming that AS1 is a superset of
> +     the generic address space).
> +
> +     If NOP_ONLY, return 'true' iff a pointer with a pointee qualified via
> +     SUBSET can be converted into a pointer with a pointee qualified via THIS
> +     (i.e. if a NOP_EXPR conversion would be valid).  In particular, this means
> +     address space mismatches are forbidden.  */
> +  bool can_qualify (qualifier_set subset, bool nop_only = false) const;

This name and the first paragraph are confusing.  What does "safely" 
mean?  If it means there's a C++ standard conversion, let's say that.

How about "superset_of"?

Also "nop_only" seems like it should be called pointee or (not_)toplevel 
or some such that describes the semantics rather than the representation.

>  static bool
> -cp_check_qualified_type (const_tree cand, const_tree base, int type_quals,
> -			 cp_ref_qualifier rqual, tree raises, bool late)
> +cp_check_qualified_type (const_tree cand, const_tree base,
> +			 cv_qualifier type_quals, cp_ref_qualifier rqual,
> +			 tree raises, bool late)
>  {
>    return (TYPE_QUALS (cand) == type_quals

Should this be TYPE_QUALS_NO_ADDR_SPACE?

> +/* Documented next to declaration in tree.h.  */

In GCC we generally document by the definition.

> -extern int cp_type_quals			(const_tree);
> -extern int type_memfn_quals			(const_tree);
> +extern cv_qualifier cp_type_quals		(const_tree);
> +extern cv_qualifier type_memfn_quals		(const_tree);

What's the rationale for the cp_type_quals having a different return 
type from TYPE_QUALS?

> +++ b/gcc/cp/cp-objcp-common.h
> @@ -142,7 +142,10 @@ static const scoped_attribute_specs *const cp_objcp_attribute_table[] =
>  #undef LANG_HOOKS_TREE_DUMP_DUMP_TREE_FN
>  #define LANG_HOOKS_TREE_DUMP_DUMP_TREE_FN cp_dump_tree
>  #undef LANG_HOOKS_TREE_DUMP_TYPE_QUALS_FN
> -#define LANG_HOOKS_TREE_DUMP_TYPE_QUALS_FN cp_type_quals
> +inline qualifier_set
> +cp_type_quals_as_set (const_tree type)
> +{ return { cp_type_quals (type) }; }
> +#define LANG_HOOKS_TREE_DUMP_TYPE_QUALS_FN cp_type_quals_as_set

This function can be defined (not inline) in cp-objc-common.cc, rather 
than the header.

Jason