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
