[Bug c/108375] New: [10/11/12/13 Regression] Some variably modified types not detected as such

[Bug c/108375] New: [10/11/12/13 Regression] Some variably modified types not detected as such

[jsm28 at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108375

            Bug ID: 108375
           Summary: [10/11/12/13 Regression] Some variably modified types
                    not detected as such
           Product: gcc
           Version: 13.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: jsm28 at gcc dot gnu.org
  Target Milestone: ---

variably_modified_type_p fails to detect an array type as variably modified if
the array and its element type are of constant size but the element type is
variably modified. For example, the following code should be diagnosed as
invalid, but is not (similar rejects-valid or wrong-code examples could no
doubt be constructed as well).

void
f (int a)
{
  typedef int A[a];
  goto x;
  A *p[2];
  x : ;
}

This is a regression in 4.2 and later relative to older versions, I think
introduced by g:2e3b8fe7b5405a94d86bfa323c0e80e83c58d784 .

commit 2e3b8fe7b5405a94d86bfa323c0e80e83c58d784
Author: Eric Botcazou <ebotcazou@adacore.com>
Date:   Wed May 17 13:11:09 2006 +0000

    tree.c (variably_modified_type_p): Return true if the element type is
variably modified without recursing.

            * tree.c (variably_modified_type_p) <ARRAY_TYPE>: Return true
            if the element type is variably modified without recursing.

    From-SVN: r113858

[Bug c/108375] [10/11/12/13 Regression] Some variably modified types not detected as such

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108375

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2023-01-11
     Ever confirmed|0                           |1
   Target Milestone|---                         |10.5
      Known to fail|                            |4.4.7
      Known to work|                            |4.1.2
             Status|UNCONFIRMED                 |NEW

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Confirmed.

[Bug c/108375] [10/11/12/13 Regression] Some variably modified types not detected as such

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108375

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
https://gcc.gnu.org/pipermail/gcc-patches/2006-May/194375.html

[Bug c/108375] [10/11/12/13 Regression] Some variably modified types not detected as such

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108375

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #2)
> https://gcc.gnu.org/pipermail/gcc-patches/2006-May/194375.html

I can't tell if the Ada testcase was added or not.

[Bug c/108375] [10/11/12/13 Regression] Some variably modified types not detected as such

[ebotcazou at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108375

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ebotcazou at gcc dot gnu.org

--- Comment #4 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> I can't tell if the Ada testcase was added or not.

gnat.dg/pointer_array.adb.  The change is probably obsolete nowadays given the
TREE_VISITED dance done in the POINTER_TYPE case.

[Bug c/108375] [10/11/12/13 Regression] Some variably modified types not detected as such

[muecker at gwdg dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108375

--- Comment #5 from Martin Uecker <muecker at gwdg dot de> ---

Recursing into arrays is simple and does not seem to cause any problems,
but this not enough for GNU C, we can also have VLA or variably modified
types as member of structs. At least the later is sometimes useful.

void bar(int a)
{
  struct foo { char (*p)[a]; };
  goto x;
  struct foo B;
  x: ;
}

Unfortunately, recursing into structs is too expensive. We could use
walk_tree_without_duplicates but this still seems expensive.

I think we should simply add a bit which is set by the C FE for such
structs.  The question is whether this is also needed by Ada or other
languages?

[Bug c/108375] [10/11/12/13 Regression] Some variably modified types not detected as such

[muecker at gwdg dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108375

--- Comment #6 from Martin Uecker <muecker at gwdg dot de> ---
Created attachment 54458
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=54458&action=edit
preliminary patch against trunk

Here is a preliminary patch. It takes a bit from type_common to cache negative
results for records / unions (i.e. not a vm-type). Performance might be ok,
although I think an approach where the FE sets the bit for records which are
known to be variaby modified and all others are not might be preferable.

[Bug c/108375] [10/11/12/13 Regression] Some variably modified types not detected as such

[muecker at gwdg dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108375

Martin Uecker <muecker at gwdg dot de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #54458|0                           |1
        is obsolete|                            |

--- Comment #7 from Martin Uecker <muecker at gwdg dot de> ---
Created attachment 54473
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=54473&action=edit
patch against trunk


This patch should be better. It is a FE only change. It stores uses a LANG_FLAG
for each type to store whether it is a VM type which is set during
construction. This should be very efficient. The existing (previously useless)
lang hook then also communicates this information to the middle end. The
variably_modified_p function in tree.cc now does unnecessary work for C, maybe
this could be avoided.

[Bug c/108375] [10/11/12/13 Regression] Some variably modified types not detected as such

[muecker at gwdg dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108375

--- Comment #8 from Martin Uecker <muecker at gwdg dot de> ---
PATCH: https://gcc.gnu.org/pipermail/gcc-patches/2023-February/612245.html

[Bug c/108375] [10/11/12/13 Regression] Some variably modified types not detected as such

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108375

--- Comment #9 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Martin Uecker <uecker@gcc.gnu.org>:

https://gcc.gnu.org/g:47821ba07a19b672d3cba351a03af2b122e02213

commit r13-6128-g47821ba07a19b672d3cba351a03af2b122e02213
Author: Martin Uecker <uecker@tugraz.at>
Date:   Wed Feb 15 10:54:00 2023 +0100

    C: Detect all variably modified types [PR108375]

    Some variably modified types were not detected correctly.
    Define C_TYPE_VARIABLY_MODIFIED via TYPE_LANG_FLAG 6 in the CFE.
    This flag records whether a type is variably modified and is
    set for all such types including arrays with variably modified
    element type or structures and unions with variably modified
    members. This is then used to detect such types in the C FE
    and middle-end (via the existing language hook).

    gcc/c/ChangeLog:
            PR c/108375
            * c-decl.cc (decl_jump_unsafe): Use c_type_variably_modified_p.
            (diagnose_mismatched_decl): Dito.
            (warn_about_goto): Dito:
            (c_check_switch_jump_warnings): Dito.
            (finish_decl): Dito.
            (finish_struct): Dito.
            (grokdeclarator): Set C_TYPE_VARIABLY_MODIFIED.
            (finish_struct): Set C_TYPE_VARIABLY_MODIFIED.
            * c-objc-common.cc (c_var_mod_p): New function.
            (c_var_unspec_p): Remove.
            * c-objc-common.h: Set lang hook.
            * c-parser.cc (c_parser_declararion_or_fndef): Use
c_type_variably_modified_p.
            (c_parser_typeof_specifier): Dito.
            (c_parser_has_attribute_expression): Dito.
            (c_parser_generic_selection): Dito.
            * c-tree.h: Define C_TYPE_VARIABLY_MODIFIED and define
c_var_mode_p.
            * c-typeck.cc: Remove c_vla_mod_p and use C_TYPE_VARIABLY_MODIFIED.

    gcc/testsuite/ChangeLog:
            PR c/108375
            * gcc.dg/pr108375-1.c: New test.
            * gcc.dg/pr108375-2.c: New test.

[Bug c/108375] [10/11/12 Regression] Some variably modified types not detected as such

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108375

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to work|                            |13.0
           Priority|P3                          |P4
            Summary|[10/11/12/13 Regression]    |[10/11/12 Regression] Some
                   |Some variably modified      |variably modified types not
                   |types not detected as such  |detected as such

--- Comment #10 from Richard Biener <rguenth at gcc dot gnu.org> ---
I assume this is now fixed for C on trunk.  Since langhooks are a no-go when
doing LTO it would be nice to have this bit in the middle-end - it should be
possible to set it when laying out the type via the langhook.

Of course what the C frontend calls "variably modified" (and the reasons it
tests that) might not be the same as what the middle-end thinks (and why
_it_ tests for it).

[Bug c/108375] [10/11/12 Regression] Some variably modified types not detected as such

[muecker at gwdg dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108375

--- Comment #11 from Martin Uecker <muecker at gwdg dot de> ---

Yes, for C this is fixed on trunk. This change seems to also fix PR102939.
There is only one place in c-common/ left where middle-end function is used, so
we could easily separate the C FE understanding of VM-types from the one of the
middle-end. I think for C++ it would also be no problem  (it even more
restrictive). Ada I have no idea.

But I suspect that different languages might also have a different idea about
this, and it would therefor make sense to decouple this.

So the question is: What is the middle-end's idea of a variably modified type?

Why does it even have to know? In contrast to types with variable size I think
a variably modified type is something which could be a FE only thing.

If LTO does not work with lang hooks (what happens? are they just removed?),
should we just remove the call to it?  (which might require the change in
c-/common).

[Bug c/108375] [11/12 Regression] Some variably modified types not detected as such

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108375

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|10.5                        |11.5

--- Comment #12 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 10 branch is being closed.