Clang warning in drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c

[Stephen Hines]

Why are there 2 different enums for this same thing at all? By casting, you
are reducing type safety in the kernel, which can cause bugs later (should
the two enums diverge in encoding). In my opinion, the proper solution is
to remove one of the enums or provide an explicit helper that does the
conversion (along with assertions for handling any unexpected cases). The
helper function should not be doing a direct cast, since a bug could change
the integer value of one (or both) of these enums so that they don't match
up.

Thanks,
Steve

On Wed, Mar 20, 2019 at 2:37 AM Koenig, Christian <Christian.Koenig at amd.com>
wrote:

> Am 20.03.19 um 05:34 schrieb Nathan Chancellor:
> > On Wed, Mar 20, 2019 at 01:31:27AM +0000, Pan, Xinhui wrote:
> >> these two enumerated types are same for now. both of them might change
> in the future.
> >>
> >> I have not used clang, but would  .block_id =  (int)head->block fix
> your warning? If such change is acceptable, I can make one then.
> >>
> >> Thanks
> >> xinhui
> >>
> >>
> >> -----Original Message-----
> >> From: Nathan Chancellor <natechancellor at gmail.com>
> >> Sent: 2019年3月20日 8:54
> >> To: Deucher, Alexander <Alexander.Deucher at amd.com>; Koenig, Christian <
> Christian.Koenig at amd.com>; Zhou, David(ChunMing) <David1.Zhou at amd.com>;
> Pan, Xinhui <Xinhui.Pan at amd.com>
> >> Cc: amd-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org;
> linux-kernel at vger.kernel.org; clang-built-linux at googlegroups.com
> >> Subject: Clang warning in drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> >>
> >> Hi all,
> >>
> >> The introduction of this file in commit dbd249c24427 ("drm/amdgpu: add
> amdgpu_ras.c to support ras (v2)") introduces the following Clang
> >> warnings:
> >>
> >> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:544:23: warning: implicit
> conversion from enumeration type 'enum amdgpu_ras_block' to different
> enumeration type 'enum ta_ras_block' [-Wenum-conversion]
> >>                          .block_id =  head->block,
> >>                                       ~~~~~~^~~~~
> >> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:545:24: warning: implicit
> conversion from enumeration type 'enum amdgpu_ras_error_type' to different
> enumeration type 'enum ta_ras_error_type' [-Wenum-conversion]
> >>                          .error_type = head->type,
> >>                                        ~~~~~~^~~~
> >> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:549:23: warning: implicit
> conversion from enumeration type 'enum amdgpu_ras_block' to different
> enumeration type 'enum ta_ras_block' [-Wenum-conversion]
> >>                          .block_id =  head->block,
> >>                                       ~~~~~~^~~~~
> >> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:550:24: warning: implicit
> conversion from enumeration type 'enum amdgpu_ras_error_type' to different
> enumeration type 'enum ta_ras_error_type' [-Wenum-conversion]
> >>                          .error_type = head->type,
> >>                                        ~~~~~~^~~~
> >> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:650:26: warning: implicit
> conversion from enumeration type 'enum amdgpu_ras_block' to different
> enumeration type 'enum ta_ras_block' [-Wenum-conversion]
> >>                  .block_id = info->head.block,
> >>                              ~~~~~~~~~~~^~~~~
> >> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:651:35: warning: implicit
> conversion from enumeration type 'enum amdgpu_ras_error_type' to different
> enumeration type 'enum ta_ras_error_type' [-Wenum-conversion]
> >>                  .inject_error_type = info->head.type,
> >>                                       ~~~~~~~~~~~^~~~
> >> 6 warnings generated.
> >>
> >> Normally, I would sent a fix for this myself but I am not entirely sure
> why these two enumerated types exist when one would do since they have the
> same values minus the prefix. In fact, the ta_ras_{block,error_type} values
> are never used aside from being defined. Some clarification would be
> appreciated.
> >>
> >> Thank you,
> >> Nathan
> > Hi Xinhui,
> >
> > Yes, explicitly casting these six spots to int would resolve this
> > warning.
>
> Another question is if it is such a good idea to just silence the warning?
>
> Maybe add a amdgpu_ras_error_to_ta() helper to do this casting?
>
> When the enums drift away from each other then we can still add warnings
> to that helper to make sure we don't accidentally cast invalid values
> around.
>
> Regards,
> Christian.
>
> >
> > Thank you for the quick response!
> > Nathan
>
> --
> You received this message because you are subscribed to the Google Groups
> "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to clang-built-linux+unsubscribe at googlegroups.com.
> To post to this group, send email to clang-built-linux at googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/clang-built-linux/63518f1f-b808-77b0-aac6-ee1ece669c4b%40amd.com
> .
> For more options, visit https://groups.google.com/d/optout.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20190320/57d57413/attachment-0001.html>