Thanks Alex and Christian,
Yup turns out [0] is not ISO C but [] is (from the googles)
C99 6.7.2.1, §16: As a special case, the last element of a structure with more
than one named member may have an incomplete array type; this is called a
flexible array member.
Either way like I said I'm not strongly motivated to change it just caught my
attention.
Cheers,
Tom
________________________________
From: Deucher, Alexander
Sent: Thursday, August 18, 2016 11:50
To: StDenis, Tom; Alex Deucher
Cc: Christian König; amd-gfx list
Subject: RE: tidy'ing up cz_hwmgr.c
IIRC, zero sized arrays are not technically allowed in C, although gcc allows
them. As I said, some versions of gcc worked, others didn't. I'm not sure
why. Also, my example was slightly wrong. atombios.h uses arrays of size 1,
not 0. So my example should look like:
struct table {
uint16_t size;
struct element elements[1];
};
atombios.h uses [1] since I don't think [0] is portable. The same indexing
issue applies to [1].
Alex
From: amd-gfx [mailto:amd-gfx-boun...@lists.freedesktop.org] On Behalf Of
StDenis, Tom
Sent: Thursday, August 18, 2016 11:40 AM
To: Alex Deucher
Cc: Christian König; amd-gfx list
Subject: Re: tidy'ing up cz_hwmgr.c
It had to be something more complicated because this demo program
#include <stdio.h>
#include <stdlib.h>
struct one {
char *foo;
int bar[0];
};
struct two {
char *foo;
int bar[1];
};
int main(void)
{
struct one *a = calloc(1, sizeof(struct one) + 4 * sizeof(int));
struct two *b = calloc(1, sizeof(struct two) + 3 * sizeof(int));
int x;
printf("a == %p\n", a);
for (x = 0; x < 4; x++)
printf("&a.bar[%d] = %p\n", x, &a->bar[x]);
printf("b == %p\n", b);
for (x = 0; x < 4; x++)
printf("&b.bar[%d] = %p\n", x, &b->bar[x]);
return 0;
}
produces this output
tom@fx8:~$ gcc test.c -o test
tom@fx8:~$ ./test
a == 0x1fd4010
&a.bar[0] = 0x1fd4018
&a.bar[1] = 0x1fd401c
&a.bar[2] = 0x1fd4020
&a.bar[3] = 0x1fd4024
b == 0x1fd4030
&b.bar[0] = 0x1fd4038
&b.bar[1] = 0x1fd403c
&b.bar[2] = 0x1fd4040
&b.bar[3] = 0x1fd4044
Which is exactly what you'd expect. I'm not strongly advocating we change the
PP code just noting it's not really clear that it's correct from a first
reading and in theory would be better with [0].
Tom
________________________________
From: Alex Deucher <alexdeuc...@gmail.com<mailto:alexdeuc...@gmail.com>>
Sent: Thursday, August 18, 2016 11:33
To: StDenis, Tom
Cc: Christian König; amd-gfx list
Subject: Re: tidy'ing up cz_hwmgr.c
The problem we ran into was when we had a struct like this:
struct table {
uint16_t size;
struct element elements[0];
};
and then we would try and index the array:
for (i = 0; i < table->size; i++) {
element = &table->elements[i];
}
element ended up off in the weeds. The only thing that seems to make some
versions of gcc happy was pointer arithmetic. E.g.,
element = (struct element *)((char *)&table->elements[0] + (sizeof(struct
element) * i));
Alex
On Thu, Aug 18, 2016 at 11:21 AM, StDenis, Tom
<tom.stde...@amd.com<mailto:tom.stde...@amd.com>> wrote:
Any modern GCC should support [0] at the tail of a struct. This came up
because when I was reading the code I saw they allocated 7 slots (plus the size
of the struct) but then fill 8 slots. It's just weird [??]
Using [0] in the struct and allocating for 8 entries makes more sense and is
clearer to read.
Tom
________________________________
From: Christian König <deathsim...@vodafone.de<mailto:deathsim...@vodafone.de>>
Sent: Thursday, August 18, 2016 11:17
To: StDenis, Tom; amd-gfx list
Subject: Re: tidy'ing up cz_hwmgr.c
Has a [1] array at the tail which is then kzalloc'ed with N-1 entries.
Shouldn't that just be a [0] with N entries allocated for clarity?
Actually the starting address of a dynamic array should be manually calculated
instead of using [1] or [0].
We had tons of problems with that because some gcc versions get this wrong and
the atombios code used this as well.
Alex how did we resolved such issues?
Regards,
Christian.
Am 18.08.2016 um 16:26 schrieb StDenis, Tom:
Tidying up cz_hwmgr.c I noted a couple of things but first is
static bool cz_dpm_check_smu_features(struct pp_hwmgr *hwmgr,
unsigned long check_feature);
Which will return "true" if the smu call fails or the feature is set.
The structure
struct phm_clock_voltage_dependency_table;
Has a [1] array at the tail which is then kzalloc'ed with N-1 entries.
Shouldn't that just be a [0] with N entries allocated for clarity?
Tom
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
