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`? 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? 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? 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? Yuxuan On Wed, Oct 8, 2025 at 7:13 AM Jason Merrill <[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 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/ > > bugzilla/show_bug.cgi?id=107187 <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 > >
