On 11/3/23 00:44, waffl3x wrote:
That leaves 2, 4, and 5.
2. I am pretty sure xobj functions should have the struct they are a
part of recorded as the method basetype member. I have already checked
that function_type and method_type are the same node type under the
hood and it does appear to be, so it should be trivial to set it.
However I do have to wonder why static member functions don't set it,
it seems to be that it would be convenient to use the same field. Can
you provide any insight into that?
method basetype is only for METHOD_TYPE; if you want the containing type
of the function, that's DECL_CONTEXT.
-- gcc/tree.h:530
#define FUNC_OR_METHOD_CHECK(T) TREE_CHECK2 (T, FUNCTION_TYPE, METHOD_TYPE)
-- gcc/tree.h:2518
#define TYPE_METHOD_BASETYPE(NODE) \
(FUNC_OR_METHOD_CHECK (NODE)->type_non_common.maxval)
The code doesn't seem to reflect that, perhaps since the underlying
node type is the same (as far as I can tell, they both inherit from
tree_type_non_common) it wasn't believed to be necessary.
Curious. It might also be a remnant of how older code dealt with how
sometimes a member function changes between FUNCTION_TYPE and
METHOD_TYPE during parsing.
Upon looking at DECL_CONTEXT though I see it seems you were thinking of
FUNCTION_DECL. I haven't observed DECL_CONTEXT being set for
FUNCTION_DECL nodes though, perhaps it should be? Although it's more
likely that it is being set and I just haven't noticed, but if that's
the case I'll have to make a note to make sure it is being set for xobj
member functions.
I would certainly expect it to be getting set already.
I was going to say that this seems like a redundancy, but I realized
that the type of a member function pointer is tied to the struct, so it
actually ends up relevant for METHOD_TYPE nodes. I would hazard a guess
that when forming member function pointers the FUNCTION_DECL node was
not as easily accessed. If I remember correctly that is not the case
right now so it might be worthwhile to refactor away from
TYPE_METHOD_BASETYPE and replace uses of it with DECL_CONTEXT.
For a pointer-to-member, the class type is part of the type, so trying
to remove it from the type doesn't sound like an improvement to me.
Specifically, TYPE_PTRMEM_CLASS_TYPE refers to TYPE_METHOD_BASETYPE for
a pointer to member function.
I'm getting ahead of myself though, I'll stop here and avoid going on
too much of a refactoring tangent. I do want this patch to make it into
GCC14 after all.
Good plan. :)
4. I have no comment here, but it does concern me since I don't
understand it at all.
In the list near the top of cp-tree.h, DECL_LANG_FLAG_6 for a
FUNCTION_DECL is documented to be DECL_THIS_STATIC, which should only be
set on the static member.
Right, I'll try to remember to check this area in the future, but yeah
that tracks because I did remove that flag. Removing that flag just so
happened to be the start of this saga of bug fixes but alas, it had to
be done.
5. I am pretty sure this is fine for now, but if xobj member functions
ever were to support virtual/override capabilities, then it would be a
problem. Is my understanding correct, or is there some other reason
that iobj member functions have a different value here?
It is surprising that an iobj memfn would have a different DECL_ALIGN,
but it shouldn't be a problem; the vtables only rely on alignment being
at least 2.
I'll put a note for myself to look into it in the future, it's an
oddity at minimum and oddities interest me :^).
There are also some differences in the arg param in
cp_build_addr_expr_1 that concerns me, but most of them are reflected
in the differences I have already noted. I had wanted to include these
differences as well but I have been spending too much time staring at
it, it's no longer productive. In short, the indirect_ref node for xobj
member functions has reference_to_this set, while iobj member functions
do not.
That's a result of difference 3.
Okay, makes sense, I'm mildly concerned about any possible side effects
this might have since we have a function_type node suddenly going
through execution paths that only method_type went through before. The
whole "reference_to_this" "pointer_to_this" thing is a little confusing
because I'm pretty sure that doesn't refer to the actual `this` object
argument or parameter since I've seen it all over the place. Hopefully
it's benign.
Yes, "pointer_to_this" is just a cache of the type that is a pointer to
the type you're looking at. You are correct that it has nothing to do
with the C++ 'this'.
The baselink binfo field has the private flag set for xobj
member functions, iobj member functions does not.
TREE_PRIVATE on a binfo is part of BINFO_ACCESS, which isn't a fixed
value, but gets updated during member search. Perhaps the differences
in consideration of conversion to a base led to it being set or cleared
differently? I wouldn't worry too much about it unless you see
differences in access control.
Unfortunately I don't have any tests for private/public access yet,
it's one of the area's I've neglected. Unfortunately I probably won't
put too much effort into writing TOO many more right now as it takes up
a lot of my time. I still have to clean up the ones I currently have
and I have a few I wanted to write that are not yet written.
Makes sense. I wouldn't expect access control to need specific changes.
I've spent too much time on this write up, so I am calling it here, it
wasn't all a waste of time because half of what I was doing here are
things I was going to need to do anyway at least. I still feel like I
spent too much time on it. Hopefully it's of some value for me/others
later.
I hope my responses are helpful as well.
Very much so, thank you!
An update on the regression testing, I had one test fail comparing
against commit a4d2b108cf234e7893322a32a7956ca24e283b05 (GCC13) and I'm
not sure if I need to be concerned about it.
libgomp.c++/../libgomp.c-c++-common/for-16.c execution test
No, that test has been pretty flaky for me, you can ignore it.
I'm going to be starting tests for my patch against trunk now, once
that is finished I should be ready to format a patch for review.
Great!
Jason
