On 9/23/24 12:58 PM, Simon Martin wrote:
Hi Jason,
On 20 Sep 2024, at 18:06, Jason Merrill wrote:
On 9/16/24 4:07 PM, Simon Martin wrote:
Hi Jason,
On 14 Sep 2024, at 18:44, Simon Martin wrote:
Hi Jason,
On 14 Sep 2024, at 18:11, Jason Merrill wrote:
On 9/13/24 11:06 AM, Simon Martin wrote:
Hi Jason,
On 12 Sep 2024, at 16:48, Jason Merrill wrote:
On 9/12/24 7:23 AM, Simon Martin wrote:
Hi,
While looking at more open PRs, I have discovered that the
problem
reported in PR109790 is very similar to that in PR100632, so
I’m
combining both in a single patch attached here. The fix is
similar
to
the one I initially submitted, only more general and I believe
better.
We currently crash upon mangling members that have an anonymous
union
or a template type.
The problem is that before calling write_unqualified_name,
write_member_name has an assert that assumes that it has an
IDENTIFIER_NODE in its hand. However it's incorrect: it has an
anonymous union in PR100632, and a template in PR109790.
The assert does not assume it has an IDENTIFIER_NODE; it assumes
it
has a _DECL, and expects its DECL_NAME to be an IDENTIFIER_NODE.
!identifier_p will always be true for a _DECL, making the assert
useless.
Indeed, my bad. Thanks for catching and explaining this!
How about checking !DECL_NAME (member) instead of !identifier_p?
Unfortunately it does not fix PR100632, that actually involves
legitimate operators.
I checked why the assert was added in the first place (via
r11-6301),
and the idea was to catch any case where we’d be missing the
“on”
marker - PR100632 contains such cases.
I assume you mean 109790?
Yes :-/
So I took the approach to refactor write_member_name a bit to
first
write the marker in all the cases required, and then actually
write
the
member name; and the assert is not needed anymore there.
Refactoring code in mangle.cc is tricky given the intent to retain
backward bug-compatibility.
Specifically, adding the "on" in ABI v11 is wrong since GCC 10 (ABI
v14-15) didn't emit it for the 109790 testcase; we can add it for
v16,
since GCC 11 ICEd on the testcase.
I would prefer to fix the bug locally rather than refactor.
Understood, that makes sense.
I’ll work on a more local patch and resubmit (by the way you can
also
ignore
https://gcc.gnu.org/pipermail/gcc-patches/2024-September/662496.html,
that is also “too wide”).
I’m attaching a revised version of the patch, that lets members
with an anonymous union type go through, and for operators,
introduces
a new ABI version under which it adds the missing "on”.
We can add the missing "on" in v16, since previous releases of v16-19
ICEd on that case.
Duh you’re right; amended in the latest revision of the patch.
@@ -3255,7 +3255,15 @@ write_member_name (tree member)
}
else if (DECL_P (member))
{
- gcc_assert (!DECL_OVERLOADED_OPERATOR_P (member));
+ if (ANON_AGGR_TYPE_P (TREE_TYPE (member)))
+ ;
+ else if (DECL_OVERLOADED_OPERATOR_P (member))
+ {
+ if (abi_check (20))
+ write_string ("on");
+ }
+ else
+ gcc_assert (identifier_p (DECL_NAME (member)));
This last else is redundant; checking DECL_OVERLOADED_OPERATOR_P
already asserts that it's an identifier.
Thanks for pointing this out, fixed in the attached revision,
successfully tested on x86_64-pc-linux-gnu. OK for trunk?
OK.
Jason
