| Message ID | 3553b1db35665e6ff08592e35eb438a574d1ad65.1725962479.git.jani.nikula@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On 11/09/2024 12:23, Jani Nikula wrote: > On Tue, 10 Sep 2024, Marc Gonzalez <mgonzalez@freebox.fr> wrote: >> On 10/09/2024 16:51, Dmitry Baryshkov wrote: >>> On Tue, Sep 10, 2024 at 01:03:43PM GMT, Jani Nikula wrote: >>> >>>> Building with clang and and W=1 leads to warning about unused >>>> pll_cmp_to_fdata(). Fix by annotating it with __maybe_unused. >>>> >>>> See also commit 6863f5643dd7 ("kbuild: allow Clang to find unused static >>>> inline functions for W=1 build"). >>>> >>>> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >>> >>> I think this function can be dropped. Marc, your call, as an author of >>> the patch? >> >> ( Why is the patch prefixed "drm/msmi", is "msmi" a typo? ) > > Whoops, a typo. > >>> For the record, Arnaud is the driver's author. >> >> pll_cmp_to_fdata() was used in hdmi_8998_pll_recalc_rate() >> in a commented code block which was later removed. >> >> Thus, yes, it is safe to completely delete the unused function. >> I'm surprised gcc didn't catch that... > > Thanks, I'll change this to drop the function. > > GCC doesn't catch unused static inlines, while Clang does. It makes no sense to me that adding "inline" would prevent GCC from diagnosing the issue... GCC should simply ignore the "inline" keyword when definition is not in a header file (maybe they don't store "origin"). Regards
On Thu, 12 Sep 2024, Marc Gonzalez <mgonzalez@freebox.fr> wrote: > On 11/09/2024 12:23, Jani Nikula wrote: >> On Tue, 10 Sep 2024, Marc Gonzalez <mgonzalez@freebox.fr> wrote: >>> On 10/09/2024 16:51, Dmitry Baryshkov wrote: >>>> On Tue, Sep 10, 2024 at 01:03:43PM GMT, Jani Nikula wrote: >>>>> See also commit 6863f5643dd7 ("kbuild: allow Clang to find unused static >>>>> inline functions for W=1 build"). [snip] >> GCC doesn't catch unused static inlines, while Clang does. > > It makes no sense to me that adding "inline" would prevent > GCC from diagnosing the issue... GCC should simply ignore > the "inline" keyword when definition is not in a header file > (maybe they don't store "origin"). Please just read the commit message for the commit I reference above for details. There's not much more I could say about it. BR, Jani.
On 12/09/2024 13:15, Jani Nikula wrote: > On Thu, 12 Sep 2024, Marc Gonzalez wrote: >> On 11/09/2024 12:23, Jani Nikula wrote: >>> On Tue, 10 Sep 2024, Marc Gonzalez wrote: >>>> On 10/09/2024 16:51, Dmitry Baryshkov wrote: >>>>> On Tue, Sep 10, 2024 at 01:03:43PM GMT, Jani Nikula wrote: >>>>>> See also commit 6863f5643dd7 ("kbuild: allow Clang to find unused static >>>>>> inline functions for W=1 build"). > > [snip] > >>> GCC doesn't catch unused static inlines, while Clang does. >> >> It makes no sense to me that adding "inline" would prevent >> GCC from diagnosing the issue... GCC should simply ignore >> the "inline" keyword when definition is not in a header file >> (maybe they don't store "origin"). > > Please just read the commit message for the commit I reference above for > details. There's not much more I could say about it. OK, I read 6863f5643dd7. My remark still stands. GCC's decision to not warn for unused static inline functions in source files (not headers) is questionable at best. (For the record, I think clang is the devil's spawn.) Regards
On Thu, Sep 12, 2024 at 02:14:10PM GMT, Marc Gonzalez wrote: > On 12/09/2024 13:15, Jani Nikula wrote: > > On Thu, 12 Sep 2024, Marc Gonzalez wrote: > >> On 11/09/2024 12:23, Jani Nikula wrote: > >>> On Tue, 10 Sep 2024, Marc Gonzalez wrote: > >>>> On 10/09/2024 16:51, Dmitry Baryshkov wrote: > >>>>> On Tue, Sep 10, 2024 at 01:03:43PM GMT, Jani Nikula wrote: > >>>>>> See also commit 6863f5643dd7 ("kbuild: allow Clang to find unused static > >>>>>> inline functions for W=1 build"). > > > > [snip] > > > >>> GCC doesn't catch unused static inlines, while Clang does. > >> > >> It makes no sense to me that adding "inline" would prevent > >> GCC from diagnosing the issue... GCC should simply ignore > >> the "inline" keyword when definition is not in a header file > >> (maybe they don't store "origin"). > > > > Please just read the commit message for the commit I reference above for > > details. There's not much more I could say about it. > > OK, I read 6863f5643dd7. > > My remark still stands. > > GCC's decision to not warn for unused static inline functions > in source files (not headers) is questionable at best. What's the difference between source file and a header after the CPP run? > (For the record, I think clang is the devil's spawn.) > > Regards >
On 12/09/2024 14:28, Dmitry Baryshkov wrote: > On Thu, Sep 12, 2024 at 02:14:10PM GMT, Marc Gonzalez wrote: >> On 12/09/2024 13:15, Jani Nikula wrote: >>> On Thu, 12 Sep 2024, Marc Gonzalez wrote: >>>> On 11/09/2024 12:23, Jani Nikula wrote: >>>>> On Tue, 10 Sep 2024, Marc Gonzalez wrote: >>>>>> On 10/09/2024 16:51, Dmitry Baryshkov wrote: >>>>>>> On Tue, Sep 10, 2024 at 01:03:43PM GMT, Jani Nikula wrote: >>>>>>>> See also commit 6863f5643dd7 ("kbuild: allow Clang to find unused static >>>>>>>> inline functions for W=1 build"). >>> >>> [snip] >>> >>>>> GCC doesn't catch unused static inlines, while Clang does. >>>> >>>> It makes no sense to me that adding "inline" would prevent >>>> GCC from diagnosing the issue... GCC should simply ignore >>>> the "inline" keyword when definition is not in a header file >>>> (maybe they don't store "origin"). >>> >>> Please just read the commit message for the commit I reference above for >>> details. There's not much more I could say about it. >> >> OK, I read 6863f5643dd7. >> >> My remark still stands. >> >> GCC's decision to not warn for unused static inline functions >> in source files (not headers) is questionable at best. > > What's the difference between source file and a header after the CPP > run? That question is moot, since the source file / header file convention exists only _before_ the preprocessor runs. If you meant to ask "How is the implementation supposed to track the origin", then I would hand wave and say "internal annotations".
On Thu, Sep 12, 2024 at 02:50:04PM GMT, Marc Gonzalez wrote: > On 12/09/2024 14:28, Dmitry Baryshkov wrote: > > On Thu, Sep 12, 2024 at 02:14:10PM GMT, Marc Gonzalez wrote: > >> On 12/09/2024 13:15, Jani Nikula wrote: > >>> On Thu, 12 Sep 2024, Marc Gonzalez wrote: > >>>> On 11/09/2024 12:23, Jani Nikula wrote: > >>>>> On Tue, 10 Sep 2024, Marc Gonzalez wrote: > >>>>>> On 10/09/2024 16:51, Dmitry Baryshkov wrote: > >>>>>>> On Tue, Sep 10, 2024 at 01:03:43PM GMT, Jani Nikula wrote: > >>>>>>>> See also commit 6863f5643dd7 ("kbuild: allow Clang to find unused static > >>>>>>>> inline functions for W=1 build"). > >>> > >>> [snip] > >>> > >>>>> GCC doesn't catch unused static inlines, while Clang does. > >>>> > >>>> It makes no sense to me that adding "inline" would prevent > >>>> GCC from diagnosing the issue... GCC should simply ignore > >>>> the "inline" keyword when definition is not in a header file > >>>> (maybe they don't store "origin"). > >>> > >>> Please just read the commit message for the commit I reference above for > >>> details. There's not much more I could say about it. > >> > >> OK, I read 6863f5643dd7. > >> > >> My remark still stands. > >> > >> GCC's decision to not warn for unused static inline functions > >> in source files (not headers) is questionable at best. > > > > What's the difference between source file and a header after the CPP > > run? > > That question is moot, since the source file / header file > convention exists only _before_ the preprocessor runs. > > If you meant to ask > "How is the implementation supposed to track the origin", > then I would hand wave and say "internal annotations". No, I asked what I meant. #include doesn't have any semantics. You can #include "source.c" in the same way. So asking the compiler to make a difference between source file and the header isn't going to work (Note, gcc has some notion of system header files and I think a pragma that changes the behaviour a bit, but we are not talking about such cases, are we?).
On Thu, 12 Sep 2024, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > On Thu, Sep 12, 2024 at 02:50:04PM GMT, Marc Gonzalez wrote: >> On 12/09/2024 14:28, Dmitry Baryshkov wrote: >> > On Thu, Sep 12, 2024 at 02:14:10PM GMT, Marc Gonzalez wrote: >> >> On 12/09/2024 13:15, Jani Nikula wrote: >> >>> On Thu, 12 Sep 2024, Marc Gonzalez wrote: >> >>>> On 11/09/2024 12:23, Jani Nikula wrote: >> >>>>> On Tue, 10 Sep 2024, Marc Gonzalez wrote: >> >>>>>> On 10/09/2024 16:51, Dmitry Baryshkov wrote: >> >>>>>>> On Tue, Sep 10, 2024 at 01:03:43PM GMT, Jani Nikula wrote: >> >>>>>>>> See also commit 6863f5643dd7 ("kbuild: allow Clang to find unused static >> >>>>>>>> inline functions for W=1 build"). >> >>> >> >>> [snip] >> >>> >> >>>>> GCC doesn't catch unused static inlines, while Clang does. >> >>>> >> >>>> It makes no sense to me that adding "inline" would prevent >> >>>> GCC from diagnosing the issue... GCC should simply ignore >> >>>> the "inline" keyword when definition is not in a header file >> >>>> (maybe they don't store "origin"). >> >>> >> >>> Please just read the commit message for the commit I reference above for >> >>> details. There's not much more I could say about it. >> >> >> >> OK, I read 6863f5643dd7. >> >> >> >> My remark still stands. >> >> >> >> GCC's decision to not warn for unused static inline functions >> >> in source files (not headers) is questionable at best. >> > >> > What's the difference between source file and a header after the CPP >> > run? >> >> That question is moot, since the source file / header file >> convention exists only _before_ the preprocessor runs. >> >> If you meant to ask >> "How is the implementation supposed to track the origin", >> then I would hand wave and say "internal annotations". > > No, I asked what I meant. #include doesn't have any semantics. You can > #include "source.c" in the same way. So asking the compiler to make a > difference between source file and the header isn't going to work (Note, > gcc has some notion of system header files and I think a pragma that > changes the behaviour a bit, but we are not talking about such cases, > are we?). Just saying, this sub-thread might be more fruitful on some GCC bug or list. BR, Jani.
On Wed, Sep 11, 2024 at 01:23:23PM GMT, Jani Nikula wrote: > On Tue, 10 Sep 2024, Marc Gonzalez <mgonzalez@freebox.fr> wrote: > > On 10/09/2024 16:51, Dmitry Baryshkov wrote: > > > >> On Tue, Sep 10, 2024 at 01:03:43PM GMT, Jani Nikula wrote: > >> > >>> Building with clang and and W=1 leads to warning about unused > >>> pll_cmp_to_fdata(). Fix by annotating it with __maybe_unused. > >>> > >>> See also commit 6863f5643dd7 ("kbuild: allow Clang to find unused static > >>> inline functions for W=1 build"). > >>> > >>> Signed-off-by: Jani Nikula <jani.nikula@intel.com> > >> > >> I think this function can be dropped. Marc, your call, as an author of > >> the patch? > > > > ( Why is the patch prefixed "drm/msmi", is "msmi" a typo? ) > > Whoops, a typo. > > > > > -> For the record, Arnaud is the driver's author. > > > > pll_cmp_to_fdata() was used in hdmi_8998_pll_recalc_rate() > > in a commented code block which was later removed. > > > > Thus, yes, it is safe to completely delete the unused function. > > I'm surprised gcc didn't catch that... > > Thanks, I'll change this to drop the function. Seeing no updated revisions here, I've posted a patch that drops the offending function. If I missed an update, please exuse me. > > GCC doesn't catch unused static inlines, while Clang does. > > BR, > Jani. > > > > > > Regards > > > > > >>> --- > >>> drivers/gpu/drm/msm/hdmi/hdmi_phy_8998.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_phy_8998.c b/drivers/gpu/drm/msm/hdmi/hdmi_phy_8998.c > >>> index 0e3a2b16a2ce..c0bf1f35539e 100644 > >>> --- a/drivers/gpu/drm/msm/hdmi/hdmi_phy_8998.c > >>> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_phy_8998.c > >>> @@ -153,7 +153,7 @@ static inline u32 pll_get_pll_cmp(u64 fdata, unsigned long ref_clk) > >>> return dividend - 1; > >>> } > >>> > >>> -static inline u64 pll_cmp_to_fdata(u32 pll_cmp, unsigned long ref_clk) > >>> +static inline __maybe_unused u64 pll_cmp_to_fdata(u32 pll_cmp, unsigned long ref_clk) > >>> { > >>> u64 fdata = ((u64)pll_cmp) * ref_clk * 10; > >>> > >>> -- > >>> 2.39.2 > > > > > > -- > Jani Nikula, Intel
On Sun, 22 Sep 2024, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > On Wed, Sep 11, 2024 at 01:23:23PM GMT, Jani Nikula wrote: >> Thanks, I'll change this to drop the function. > > Seeing no updated revisions here, I've posted a patch that drops the > offending function. If I missed an update, please exuse me. Thanks, I just didn't get around to it yet. BR, Jani.
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_phy_8998.c b/drivers/gpu/drm/msm/hdmi/hdmi_phy_8998.c index 0e3a2b16a2ce..c0bf1f35539e 100644 --- a/drivers/gpu/drm/msm/hdmi/hdmi_phy_8998.c +++ b/drivers/gpu/drm/msm/hdmi/hdmi_phy_8998.c @@ -153,7 +153,7 @@ static inline u32 pll_get_pll_cmp(u64 fdata, unsigned long ref_clk) return dividend - 1; } -static inline u64 pll_cmp_to_fdata(u32 pll_cmp, unsigned long ref_clk) +static inline __maybe_unused u64 pll_cmp_to_fdata(u32 pll_cmp, unsigned long ref_clk) { u64 fdata = ((u64)pll_cmp) * ref_clk * 10;
Building with clang and and W=1 leads to warning about unused pll_cmp_to_fdata(). Fix by annotating it with __maybe_unused. See also commit 6863f5643dd7 ("kbuild: allow Clang to find unused static inline functions for W=1 build"). Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- Cc: Rob Clark <robdclark@gmail.com> Cc: Abhinav Kumar <quic_abhinavk@quicinc.com> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Cc: Sean Paul <sean@poorly.run> Cc: Marijn Suijten <marijn.suijten@somainline.org> Cc: linux-arm-msm@vger.kernel.org Cc: Nathan Chancellor <nathan@kernel.org> --- drivers/gpu/drm/msm/hdmi/hdmi_phy_8998.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)