This is a summary of discussions relative to the merge request created by Guillaume Gomez (imperio) <[email protected]> titled Merge common attribute handling code between libgccjit and `gcc/c-family` since its creation.
Description: This is my third attempt at removing duplicated code between `libgccjit` and `gcc/c-family` and it seems like I was able to make it work with the help of @antoyo for the Makefile files in particular. So this is what I did: moving code from `gcc/c-family.cc` into `gcc/attr-handlers.cc`, then I remove the duplicated code in `jit/dummy-frontend.cc`. There is one exception though: since libgccjit implementation of the `malloc` attribute is a sub-part of the `gcc/c-family.cc` implementation, I created a new `handle_malloc_common` function which is called directly by libgccjit and called under a specific condition in `gcc/c-family.cc`. There are still duplicated code attributes remaining in libgccjit, but the patch is already big enough as it is so I stopped at this point. CC: David Malcolm <[email protected]>, <[email protected]>, <[email protected]> The full and up to date discussion can be found at https://forge.sourceware.org/gcc/gcc/pulls/207 The merge request has been closed without being merged directly on the forge repository. On 2026-07-30 16:03:13+00:00, Guillaume Gomez (imperio) wrote: Finally fixed formatting (from old code mostly ^^'). On 2026-08-18 22:30:18+00:00, David Malcolm (dmalcolm) wrote: The overall idea looks good to me from a libgccjit maintainer point-of-view, though I didn't painstakingly check every hunk of the move. @jsm28 : what do you think from a C maintainer/global review POV? On 2026-08-30 19:38:39+00:00, Guillaume Gomez (imperio) wrote: ping @jsm28 On 2026-09-01 16:26:31+00:00, Joseph Myers (jsm28) wrote: The principle seems fine but the forge doesn't provide an obvious way to compare the moved code (the old version of c-attribs.cc with the new attr-handlers.cc file), and since CI is reporting conflicts with c-attribs.cc, clearly this pull request would need updating including updating the moved copy of any moved code that has changed in c-attribs.cc since work on this pull request started. On 2026-09-01 20:06:57+00:00, Guillaume Gomez (imperio) wrote: I'll take that as you globally approving the patch. Considering that it moves a lot of code around, it's very prone to bitrot. I'm fixing the merge conflict, but if another merge conflict happens before next review, I'll just wait for you to review the broken state as it's time consuming to keep up. On 2026-09-01 20:18:21+00:00, Guillaume Gomez (imperio) wrote: Fixed merge conflict. Ping @jsm28 For the review, I can only recommend to check that the function in `attr-handlers.cc` has the same code as the one removed from `c-attribs.cc`. Not too sure how to make the review easier for you, though. Sorry. :-/ On 2026-09-16 15:46:56+00:00, Arthur Cohen (cohenarthur) <[email protected]> approved the changes: The code changes look good to me. Some of these functions will also be useful to the Rust frontend, so I'm in favor of the move and would like to see it merged. On 2026-09-21 12:40:51+00:00, Guillaume Gomez (imperio) wrote: Ping @jsm28 On 2026-09-23 21:12:27+00:00, Joseph Myers (jsm28) wrote: Breaking a line after `DECL_P (` in the relocated `validate_attr_args` is not the normal style used. Other than that reformatting, the move of C front-end code is OK. Note that I have not reviewed functions relocated from the JIT front end such as `handle_transaction_pure_attribute`. On 2026-09-24 09:33:12+00:00, Guillaume Gomez (imperio) wrote: Thanks! Then ping @dmalcolm :) On 2026-09-30 17:18:03+00:00, David Malcolm (dmalcolm) <[email protected]> approved the changes: Thanks for the patch. Note that `handle_transaction_pure_attribute` is only used by libgccjit (c-attribs.cc uses `handle_tm_attribute`) so this probably shouldn't be in the shared file. The patch introduces some minor behavior changes to libgccjit for the following, but I believe only `"pure"`, `"const"` and `"returns_twice"` are currently reachable (based on `fn_attribute_to_string` in jit-playback.cc) * "pure" in libgccjit with a non-FUNCTION_DECL used to ICE with gcc_unreachable but now emits a warning. Would be an improvement. * "pure" in libgccjit with a void-returning function (e.g. via GCC_JIT_FN_ATTRIBUTE_PURE) now warns when it didn't before. This could lead to previously-compiling gcc_jit_contexts failing after the patch. A behavior change, but I don't think it's a problem. * "const" in libgccjit on a void-returning function (e.g. via GCC_JIT_FN_ATTRIBUTE_CONST): gains a warning, so a similar behavior change. I think it's OK, for the same reasons. * "returns_twice" in libgccjit on a non-FUNCTION-DECL used to ICE with gcc_unreachable but now emits a warning (an improvement); not reachable from public jit API * "nothrow" same as "returns_twice", but reachable from API for builtins dummy-frontend.cc can lose its definition of ATTR_EXCL after this patch (but we can defer this to a followup). So I think this is OK from my POV, but @jsm28 might want you to drop the move of `handle_transaction_pure_attribute`. Patch is probably good enough as-is if this has already been tested thoroughly (bootstrap + make check-jit; check-gcc/check-c++), given how much of a pain this is causing you. Hence marking as "approved" but note the above. On 2026-09-30 18:30:53+00:00, Guillaume Gomez (imperio) wrote: Moved `handle_transaction_pure_attribute` back to `dummy-frontend.cc`. On 2026-09-30 18:33:14+00:00, Guillaume Gomez (imperio) wrote: Thanks for the review @dmalcolm ! I re-ran the testsuites (jit, C++ and C front-ends) and no new issue. On 2026-09-30 19:12:32+00:00, Antoni Boucher (antoyo) commented on the code: > +++ gcc/jit/dummy-frontend.cc > @@ -494,3 +279,3 @@ > /* Handle a "transaction_pure" attribute. */ > > -static tree > +tree Please add back the `static` here. > +++ gcc/jit/dummy-frontend.cc > @@ -494,3 +279,3 @@ > /* Handle a "transaction_pure" attribute. */ > > -static tree > +tree Done! On 2026-10-01 14:08:18+00:00, Antoni Boucher (antoyo) commented on the code: > +++ gcc/attr-handlers.cc > @@ -0,0 +204,4 @@ > + the previous one. */ > + const location_t curloc = input_location; > + const location_t prevloc = DECL_P ( > + node[1]) ? DECL_SOURCE_LOCATION (node[1]) : curloc; The format checker doesn't error on this, but the original formatting was: ```c const location_t prevloc = DECL_P (node[1]) ? DECL_SOURCE_LOCATION (node[1]) : curloc; ``` > +++ gcc/attr-handlers.cc > @@ -0,0 +204,4 @@ > + the previous one. */ > + const location_t curloc = input_location; > + const location_t prevloc = DECL_P ( > + node[1]) ? DECL_SOURCE_LOCATION (node[1]) : curloc; Done! On 2026-10-01 17:49:26+00:00, Antoni Boucher (antoyo) approved the changes: On 2026-10-02 09:49:18+00:00, Guillaume Gomez (imperio) wrote: It was merged by @antoyo as commit 54df3a3d98b521ad0652b2fe23d9132bcf77d0fa. Closing then. Thanks everyone! I'll send follow-ups.
