#2102 Instances of structs with flexible arrays

[Philipp Klaus Krause]

There is the regression test gcc-torture-execute-20050613-1.c, which uses a flexible array member in a struct. This is allowed in a struct declaration in C99, but it makes the type of the struct incomplete. Thus creating an instance of it is not allowed (structs with flexible arrays are meant to be allocated using malloc()). Popular compilers, such as gcc, allow creating an instance, and treat the flexible length array as zero-length array.
It seems sdcc works the same, but does not give a warning, even for --std-c99.

I see multiple ways to handle this issue:
1) Give a warning.
2) Disallow instances of structs with flexible arrays, i.e. give an error.

Philipp

[Ben Shi]

A warning is added for flexible arrays in a struct, in reversion #9191.

[Maarten Brock]

This fix is wrong. It triggers on the declaration instead of on a definition (creating an instance). I will disable it and have reopened this bug.

[Ben Shi]

if we warn each creation of an instance than the definition , can the warnings become even more?

since one struct definition may correspond to several instance creations.

[Philipp Klaus Krause]

The code could still use the declaration for pointers to the incomplete struct. This is okay and should not raise a warning.
AFAIK, the correct way to use these structs from the programmer perspective is to malloc() the needed space and work with pointers to the struct. This use should not result in warnings.

Philipp

[Maarten Brock]

And every creation of such an instance is a violation of instantiating an incomplete type and thus deserves a warning.

Currently our malloc.c, calloc.c and free.c use this feature and they do not instantiate, but only use pointers to such a type. The current code does however throw warnings when building them and it should not.

[Ben Shi]

I see . I misunderstood the use of flexible arrays.

[Philipp Klaus Krause]

However, since flexible arrays in declarations are a C99 feature, we should still warn on the declaration when compiling with --std-c89 or --std-sdcc89.

Philipp

[Maarten Brock]

Not if that means that every pic user is obliged to use c99 or higher or else suffer the warnings that we caused.

[Ben Shi]

Fixed in reversion 9228.

1. If there is definition of a struct with a flexible array, warning 187 will rise in --std-c89, but won't in --std-c99 and --std-c11.

2. If there is a definition of a variable of a struct with a flexible array, warning 219 will rise, but won't if such a pointer is defined.

[Maarten Brock]

Warning 219 triggers on an extern declaration in device/lib/pic14/libsdcc/idata.c (line 62). I'm unclear if this should be treated the same as a definition or as a pointer definition, but I think like the latter since it doesn't allocate memory. As such it should not produce a warning.

[Ben Shi]

how about change warning message to "struct with flexible array field should only access via pointer"？ though no memory is allocated, i thought it is still an illegal use, the allocation still be in another place.

[Maarten Brock]

If the allocation happens in another place, then the warning should appear there, I think. Further, the definition might use a complete type, like this:

struct {
    unsigned records;    // number of entries in this file
    cinit_t  entry[10];  // intialization descriptor
} cinit;

Or as in the case of idata.c, the linker will create it.

But I admit, this is just my opinion and I would like to hear others opinions as well.

I noticed you also added a regression test which triggers the warning. Was it your intention to check this warning or just the behaviour? I would prefer to suppress the warning with a pragma here. Further, the asserts seem wrong as they are assignments.

[Ben Shi]

My opinion: A warning means the code might be wrong, but not must be wrong.

Though your case is right, we can not assume other users can also correctly do.

For the assignment, it my typo, and I will correct it soon.

[Maarten Brock]

I agree that a warning should indicate the code might be wrong. But we should try hard to keep the number of false positives down. If you've ever worked with Xilinx VHDL tools, you would know how bad this can be. The important warnings get to totally drowned in 1000's of unimportant ones.

Thanks for updating the regression test. However, I fear it will now fail on systems that do not pack structures. I think you should keep a, b, c & p int.

[Ben Shi]

1. In reversion 9229, I added a commented line 569 in SDCC.y,

569: // if (l0 == NULL && l1 != NULL && SPEC_EXTR($1) != 1)
570: if (l0 == NULL && l1 != NULL)

you can easily turn off the warning on declaration (with extern) while keep the warning on definition (without extern).
by uncommenting line 569 and commenting the line 570.

1. I am OK with changing the bug-2102.c according to your struct packing concern.

And could you please do the above changes and have a full reg-test? I am occupied by an urgent event of my customer till this weekend.

[Maarten Brock]

Sure, no problem.