| Message ID | 1395766541-23979-6-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Accepted, archived |
| Headers | show |
On 03/25/2014 12:55 PM, Julien Grall wrote: > Fix compilation with Clang 3.5: > > avc.c:657:19: error: unused function 'avc_sidcmp' [-Werror,-Wunused-function] > static inline int avc_sidcmp(u32 x, u32 y) > ^ > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov> Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>>> On 25.03.14 at 17:55, <julien.grall@linaro.org> wrote: > Fix compilation with Clang 3.5: > > avc.c:657:19: error: unused function 'avc_sidcmp' [-Werror,-Wunused-function] > static inline int avc_sidcmp(u32 x, u32 y) Despite Daniel having acked this already, this seems conceptually wrong to me: static inline functions are quite frequently unused (and I assume the compiler warns about them only if they're not in a header file), and hence the compiler shouldn't be warning about them in the first place. Jan
On 03/26/2014 11:57 AM, Jan Beulich wrote: >>>> On 25.03.14 at 17:55, <julien.grall@linaro.org> wrote: >> Fix compilation with Clang 3.5: >> >> avc.c:657:19: error: unused function 'avc_sidcmp' [-Werror,-Wunused-function] >> static inline int avc_sidcmp(u32 x, u32 y) > > Despite Daniel having acked this already, this seems conceptually wrong > to me: static inline functions are quite frequently unused (and I assume > the compiler warns about them only if they're not in a header file), and > hence the compiler shouldn't be warning about them in the first place. This function has not been used seen 2007. I think this is the only static inline function not defined in headers which is not used. Why shouldn't the compiler warn about it? Rather than static inline function defined in the header, this kind function is dead code. If we want to keep it we should at least mark them as unused. IHMO I don't think we need to keep function that weren't used since ages and I bet it was a forgotten when the code was reworked a long time ago. Regards,
>>> On 26.03.14 at 17:11, <julien.grall@linaro.org> wrote: > On 03/26/2014 11:57 AM, Jan Beulich wrote: >>>>> On 25.03.14 at 17:55, <julien.grall@linaro.org> wrote: >>> Fix compilation with Clang 3.5: >>> >>> avc.c:657:19: error: unused function 'avc_sidcmp' [-Werror,-Wunused-function] >>> static inline int avc_sidcmp(u32 x, u32 y) >> >> Despite Daniel having acked this already, this seems conceptually wrong >> to me: static inline functions are quite frequently unused (and I assume >> the compiler warns about them only if they're not in a header file), and >> hence the compiler shouldn't be warning about them in the first place. > > This function has not been used seen 2007. I think this is the only > static inline function not defined in headers which is not used. > > Why shouldn't the compiler warn about it? Rather than static inline > function defined in the header, this kind function is dead code. If we > want to keep it we should at least mark them as unused. > > IHMO I don't think we need to keep function that weren't used since ages > and I bet it was a forgotten when the code was reworked a long time ago. Yes, and my comment wasn't about this specific function, but the general pattern: You justified your change with the build breakage, whereas you could have stated what you said just now in your reply. IOW I'm fine with you cleaning up things that were more or less obviously forgotten in an earlier change. But code adjustments just to satisfy an overly picky compiler aren't that nice. After all such functions can serve a purpose (and I think if you looked around you'd find other unused stuff that could be deleted yet - so far - isn't being, as being potentially useful in the future), and the compiler here - from what I can tell - is warning on these simply because they aren't in header files. Which in turn raises the question how the compiler knows what a header file is (remember that we have quite a few instances of .c files including other .c files, and I'd bet the compiler treats these as header files too). Summary: Likely the compiler is issuing this sort of warning inconsistently, and hence it would better not issue the warning at all (or at least provide a way to suppress it). Jan
On 03/26/2014 04:42 PM, Jan Beulich wrote: >>>> On 26.03.14 at 17:11, <julien.grall@linaro.org> wrote: >> On 03/26/2014 11:57 AM, Jan Beulich wrote: >>>>>> On 25.03.14 at 17:55, <julien.grall@linaro.org> wrote: >>>> Fix compilation with Clang 3.5: >>>> >>>> avc.c:657:19: error: unused function 'avc_sidcmp' [-Werror,-Wunused-function] >>>> static inline int avc_sidcmp(u32 x, u32 y) >>> >>> Despite Daniel having acked this already, this seems conceptually wrong >>> to me: static inline functions are quite frequently unused (and I assume >>> the compiler warns about them only if they're not in a header file), and >>> hence the compiler shouldn't be warning about them in the first place. >> >> This function has not been used seen 2007. I think this is the only >> static inline function not defined in headers which is not used. >> >> Why shouldn't the compiler warn about it? Rather than static inline >> function defined in the header, this kind function is dead code. If we >> want to keep it we should at least mark them as unused. >> >> IHMO I don't think we need to keep function that weren't used since ages >> and I bet it was a forgotten when the code was reworked a long time ago. > > Yes, and my comment wasn't about this specific function, but the > general pattern: You justified your change with the build breakage, > whereas you could have stated what you said just now in your > reply. IOW I'm fine with you cleaning up things that were more or > less obviously forgotten in an earlier change. But code adjustments > just to satisfy an overly picky compiler aren't that nice. After all > such functions can serve a purpose (and I think if you looked around > you'd find other unused stuff that could be deleted yet - so far - isn't > being, as being potentially useful in the future), and the compiler here > - from what I can tell - is warning on these simply because they aren't > in header files. Which in turn raises the question how the compiler > knows what a header file is (remember that we have quite a few > instances of .c files including other .c files, and I'd bet the compiler > treats these as header files too). Summary: Likely the compiler is > issuing this sort of warning inconsistently, and hence it would better > not issue the warning at all (or at least provide a way to suppress it). Thanks for your comment. I will update the different commit message. The new version of clang has amazing feature like guard checking (see patch #17)... I was wondering the same thing when I first discovered this kind of errors. I guess with the '# 1 "/local/.../.h" hints the compiler is able to know where does the error come from. I will try to find why clang is considering static inline invalid in .c context.
On 03/26/2014 05:06 PM, Julien Grall wrote: > On 03/26/2014 04:42 PM, Jan Beulich wrote: >>>>> On 26.03.14 at 17:11, <julien.grall@linaro.org> wrote: >>> On 03/26/2014 11:57 AM, Jan Beulich wrote: >>>>>>> On 25.03.14 at 17:55, <julien.grall@linaro.org> wrote: >>>>> Fix compilation with Clang 3.5: >>>>> >>>>> avc.c:657:19: error: unused function 'avc_sidcmp' [-Werror,-Wunused-function] >>>>> static inline int avc_sidcmp(u32 x, u32 y) >>>> >>>> Despite Daniel having acked this already, this seems conceptually wrong >>>> to me: static inline functions are quite frequently unused (and I assume >>>> the compiler warns about them only if they're not in a header file), and >>>> hence the compiler shouldn't be warning about them in the first place. >>> >>> This function has not been used seen 2007. I think this is the only >>> static inline function not defined in headers which is not used. >>> >>> Why shouldn't the compiler warn about it? Rather than static inline >>> function defined in the header, this kind function is dead code. If we >>> want to keep it we should at least mark them as unused. >>> >>> IHMO I don't think we need to keep function that weren't used since ages >>> and I bet it was a forgotten when the code was reworked a long time ago. >> >> Yes, and my comment wasn't about this specific function, but the >> general pattern: You justified your change with the build breakage, >> whereas you could have stated what you said just now in your >> reply. IOW I'm fine with you cleaning up things that were more or >> less obviously forgotten in an earlier change. But code adjustments >> just to satisfy an overly picky compiler aren't that nice. After all >> such functions can serve a purpose (and I think if you looked around >> you'd find other unused stuff that could be deleted yet - so far - isn't >> being, as being potentially useful in the future), and the compiler here >> - from what I can tell - is warning on these simply because they aren't >> in header files. Which in turn raises the question how the compiler >> knows what a header file is (remember that we have quite a few >> instances of .c files including other .c files, and I'd bet the compiler >> treats these as header files too). Summary: Likely the compiler is >> issuing this sort of warning inconsistently, and hence it would better >> not issue the warning at all (or at least provide a way to suppress it). > > Thanks for your comment. I will update the different commit message. > > The new version of clang has amazing feature like guard checking (see > patch #17)... I was wondering the same thing when I first discovered > this kind of errors. > > I guess with the '# 1 "/local/.../.h" hints the compiler is able to know > where does the error come from. > > I will try to find why clang is considering static inline invalid in .c > context. > Here we go, this has been explicitly added in clang a while ago: commit 39bd371610af27b073c792c54c6c28133329f6ad Date: Tue Sep 10 03:05:56 2013 +0000 Make -Wunused warning rules more consistent. This patch does a few different things. This patch improves unused var diags for const vars: we no longer unconditionally suppress diagnostics for const vars, instead only suppressing the diagnostic when the declaration appears to be useful. This patch also makes us more consistently use whether a variable/function is declared in the main file to suppress diagnostics where appropriate. Fixes <rdar://problem/14907887>. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@190382 91177308-0d34-0410-b5e6-96231b3b80d8
diff --git a/xen/xsm/flask/avc.c b/xen/xsm/flask/avc.c index a26b34a..fc6580e 100644 --- a/xen/xsm/flask/avc.c +++ b/xen/xsm/flask/avc.c @@ -654,11 +654,6 @@ int avc_add_callback(int (*callback)(u32 event, u32 ssid, u32 tsid, u16 tclass, return rc; } -static inline int avc_sidcmp(u32 x, u32 y) -{ - return (x == y || x == SECSID_WILD || y == SECSID_WILD); -} - /** * avc_update_node Update an AVC entry * @event : Updating event
Fix compilation with Clang 3.5: avc.c:657:19: error: unused function 'avc_sidcmp' [-Werror,-Wunused-function] static inline int avc_sidcmp(u32 x, u32 y) ^ Signed-off-by: Julien Grall <julien.grall@linaro.org> Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov> --- xen/xsm/flask/avc.c | 5 ----- 1 file changed, 5 deletions(-)