tidy'ing up cz_hwmgr.c

Thu Aug 18 16:07:02 UTC 2016

> Either way like I said I'm not strongly motivated to change it just 
> caught my attention.
>
Well if you have time it would be really cool if you could a) identify 
such cases before we run into issues with some gcc versions again and b) 
leave the people who added the code and/or are responsible for this part 
of the driver a note to fix it.

We really had a headache with those things because of the coding style 
in the atom headers and I would really like to avoid that in the future.

Thanks in advance,
Christian.

Am 18.08.2016 um 17:53 schrieb StDenis, Tom:
>
> 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-bounces at 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 at fx8:~$ gcc test.c -o test
>
> tom at 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 <alexdeucher at gmail.com <mailto:alexdeucher at 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.StDenis at amd.com 
> <mailto:Tom.StDenis at 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 <deathsimple at vodafone.de 
> <mailto:deathsimple at 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 at lists.freedesktop.org <mailto:amd-gfx at lists.freedesktop.org>
>
>     https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org <mailto:amd-gfx at lists.freedesktop.org>
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20160818/bb2bfcb0/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: image/png
Size: 488 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20160818/bb2bfcb0/attachment-0001.png>