Hi Jason,
On Thu Apr 10, 2025 at 10:42 PM CEST, Jason Merrill wrote:
> On 9/6/24 7:15 AM, Simon Martin wrote:
>> We ICE upon the following *valid* code when mangling the requires
>> clause
>>
>> === cut here ===
>> template <int> struct s1 {
>> enum { e1 = 1 };
>> };
>> template <int t2> struct s2 {
>> enum { e1 = s1<t2>::e1 };
>> s2() requires(0 != e1) {}
>> };
>> s2<8> a;
>> === cut here ===
>>
>> The problem is that the mangler wrongly assumes that the DECL_INITIAL of
>> a CONST_DECL is always an INTEGER_CST, and blindly passes it to
>> write_integer_cst.
>>
>> I assume we should be able to actually compute the value of e1 and use
>> it when mangling, however from my investigation, it seems to be a pretty
>> involved change.
>>
>> What's clear however is that we should not try to write a non-literal as
>> a literal. This patch adds a utility function to determine whether a
>> tree is a literal as per the definition in the ABI, and uses it to only
>> call write_template_arg_literal when we actually have a literal in hand.
>>
>> Note that I had to change the expectation of an existing test, that was
>> expecting "[...]void (AF::*)(){}[...]" and now gets an equivalent
>> "[...](void (AF::*)())0[...]" (and FWIW is what clang and icx give; see
>> https://godbolt.org/z/hnjdeKEhW).
>
> Unfortunately we need to provide backward bug compatibility for
> -fabi-version=14, so this change needs to check abi_version_at_least (15).
Good point, ack.
>> +/* Determine whether T is a literal per section 5.1.6.1 of the CXX ABI. */
>> +
>> +static bool
>> +literal_p (const tree t)
>> +{
>> + if ((TREE_TYPE (t) && NULLPTR_TYPE_P (TREE_TYPE (t)))
>
> This looks wrong; a random expression with type nullptr_t is not a
> literal, and can be instantiation-dependent. And I don't see any test
> of mangling such a thing.
TBH I think there might be more than just this wrong with this patch :-)
I have been flip-flopping between "it's wrong to just mangle the
expression" and "but I don't think we can do much better" and never
settled on one; that's why I never pinged this 6+ month old patch.
Is the approach this took actually valid? I think that in an ideal
world, the enum value would have been tsubst'd (or we'd have all we need
to tsubst it) when we mangle, and I tried to hook things up so that it
happens, but I never succeeded.
Simon
