On 10/8/25 7:03 PM, Yuxuan Chen wrote:
Hi Jason,
Thank you for your prompt review. I will try to do as suggested. I have
several questions:
1. For the legal requirements, most likely I will need to use the
Developer Certificate of Origin. Does that mean I will just need to do
`git commit --signoff`?
If you can assert the terms of the DCO then yes, that command will add
the Signed-off-by.
2. Are there any easy ways to format the code? Due to the unfamiliarity
I have with the GNU style, conforming to the coding standards takes a
long time. For example, my editor is not well set up to take a mixture
of tabs and spaces. Is it typical for contributors to manually format
the code or is there a script I am missing?
I use Emacs, which follows the coding style by default; I'm not very
familiar with what's needed for other editors.
Which editor are you using? Is it possible for it to use the
.editorconfig file in the top-level source directory?
See also contrib/clang-format for how to use clang-format-diff to adjust
the formatting in your patch; it also seems that there is a
git-clang-format tool. I haven't used these, myself.
3. I tried to run all existing tests with `make && make -C gcc -k check`
and there are a small quantity of existing (unexpected) failures. Is
that an indication that I don't have the right setup?
Unfortunately, sometimes there are some accidental testsuite failures
that persist for a while until they get fixed; this is why the
contributing page suggests comparing pre- and post-patch test results.
I tend to run a nightly baseline with a cron job and compare against that.
It would be good for us to be stricter about no unexpected failures on
primary targets, to reduce this overhead and confusion. There is
automatic testing now that will tell people that their patch (or another
patch that went in around the same time) caused a new failure, but we
currently don't go beyond that to guarantee no unexpected failures.
4. For subsequent submissions after these modifications, can I just
compute a new patch and attach it in a reply-all to this email chain?
That's fine, at least currently.
Jason
On Wed, Oct 8, 2025 at 7:13 AM Jason Merrill <[email protected]
<mailto:[email protected]>> wrote:
On 10/7/25 11:34 PM, Yuxuan Chen wrote:
> Hello,
>
> First attempt at contributing to GCC. My GCC knowledge is limited
so any
> assistance would be appreciated.
Thanks, this is a great start!
Please look over https://gcc.gnu.org/contribute.html <https://
gcc.gnu.org/contribute.html> for more guidance
about patch submission, particularly legal prerequisites and ChangeLog
entries.
> I have tried to implement a `trivial_abi` attribute for GCC
because I
> ran into an issue interacting with clang built code where
`trivial_abi`
> was in use. Without implementing the same attribute the same way, we
> risk having nuanced calling convention mismatches.
>
> There also has been a long standing feature request https://
gcc.gnu.org/ <https://gcc.gnu.org/>
> bugzilla/show_bug.cgi?id=107187 <https://gcc.gnu.org/bugzilla/
<https://gcc.gnu.org/bugzilla/>
> show_bug.cgi?id=107187>
>
> I have attempted to get the patch accepted by the formatter.
However, I
> didn't fix the one remaining "There should be exactly one space
between
> function name and parenthesis." following the code surrounding my
change.
> @@ -2527,6 +2527,8 @@ struct GTY(()) lang_type {
> bool replaceable : 1;
> bool replaceable_computed : 1;
>
> + bool has_trivial_abi : 1;
Hmm, do we need a bit for this? I think we could lookup_attribute in
finish_struct_bits and otherwise rely on TREE_ADDRESSABLE which is the
flag that actually controls whether a type is passed by invisible
reference.
> +trivial_for_calls_p (const_tree t)
> +{
> + t = strip_array_types (CONST_CAST_TREE (t));
> +
> + if (trivial_type_p (t))
> + return true;
I think we don't need this function, but if we do, trivial_type_p is
wrong; for calls we don't care whether the default constructor is
trivial.
> + /* trivial_abi attribute makes the class trivially
relocatable. */
Hmm, that seems a bit arbitrary but I see it's the clang behavior.
> @@ -5602,6 +5626,8 @@ static const attribute_spec
cxx_gnu_attributes[] =
> handle_abi_tag_attribute, NULL },
> { "no_dangling", 0, 1, false, true, false, false,
> handle_no_dangling_attribute, NULL },
> + { "trivial_abi", 0, 0, false, true, false, true,
> + handle_trivial_abi_attribute, NULL },
I assume we also want to support [[clang::trivial_abi]]. We already
have c_common_clang_attributes, but perhaps we need to add
cxx_clang_attributes as well.
> + /* Check for virtual bases. */
> + if (TYPE_BINFO (type) && BINFO_N_BASE_BINFOS (TYPE_BINFO
(type)) > 0)
It's simpler to check CLASSTYPE_VBASECLASSES for this.
> + "%qE attribute ignored on type with virtual
methods", name);
C++ doesn't use the term "method", so let's say "virtual functions".
> + if (!trivial_for_calls_p (base_type))
> + {
> + warning (OPT_Wattributes,
> + "%qE attribute ignored on type with non-
trivial base"
> + " class %qT", name, base_type);
As I was suggesting above, we can check TREE_ADDRESSABLE instead of
trivial_for_calls_p.
> + tree field_type = TREE_TYPE (field);
> +
> + /* Handle array types. */
> + while (TREE_CODE (field_type) == ARRAY_TYPE)
> + field_type = TREE_TYPE (field_type);
You can use strip_array_types here as well.
> + /* Check for non-deleted copy constructor. */
> + if (TYPE_HAS_COPY_CTOR (type) && !
TYPE_HAS_COMPLEX_COPY_CTOR (type))
> + has_non_deleted_copy_or_move = true;
I don't think this covers the case of a lazily declared and non-deleted
but non-trivial copy constructor.
> + /* Check for non-deleted move constructor. */
> + if (classtype_has_non_deleted_move_ctor (type))
> + has_non_deleted_copy_or_move = true;
>
> + /* Check declared constructors. */
> + if (CLASSTYPE_CONSTRUCTORS (type))
> + {
> + for (ovl_iterator iter (CLASSTYPE_CONSTRUCTORS (type));
iter; ++iter)
> + {
> + tree fn = *iter;
> + if ((copy_fn_p (fn) || move_fn_p (fn)) && !
DECL_DELETED_FN (fn))
> + {
> + has_non_deleted_copy_or_move = true;
> + break;
> + }
> + }
> + }
Let's factor out a classtype_has_non_deleted_copy_or_move_ctor that
lazily declares the copy/move ctors and then does the loop.
> \ No newline at end of file
Please add ending newlines to the testcases.
Might be worth testing that this attribute doesn't affect "trivially
copyable".
The tests don't seem to actually check the parameter passing/return
ABI.
See the existing g++.dg/abi/invisiref* tests for how to check
whether
arguments are passed by invisible reference or not.
We also need to check whether (how many times) side-effects from
non-trivial [cd]tors still occur.
The attribute needs documentation in the "C++ Attributes" section of
gcc/doc/extend.texi.
Thanks again,
Jason
