Bug 68230

Summary: Unused function parameters not reported by -Wunused-parameter when only used recursively (add -Wparameter-only-used-recursively instead?)
Product: gcc Reporter: Joshua T, Fisher <j.fisher>
Component: c++Assignee: Not yet assigned to anyone <unassigned>
Status: NEW ---    
Severity: enhancement CC: dimhen, egallager, jamborm, webrown.cpp
Priority: P3 Keywords: diagnostic
Version: 5.2.0   
Target Milestone: ---   
See Also: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=32562
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94693
Host: Target:
Build: Known to work:
Known to fail: Last reconfirmed: 2017-09-28 00:00:00
Bug Depends on:    
Bug Blocks: 87403, 89180    

Description Joshua T, Fisher 2015-11-06 01:11:35 UTC
The code below does not produce a warning when -Wunused-parameter is turned on. It would be fantastic if it did, as fundamentally, the variable notUsed is still not actually being used.

#include <iostream>
using namespace std;

int Recursive(int num, const char *notUsed);

int main() {
  const char *notUsedLocal = "Not used";

  cout << "Test: " << Recursive(1, notUsedLocal) << endl;
  return 0;
}

int Recursive(int num, const char *notUsed) {
  if (num > 5)
    return num;
  else
    return Recursive(++num, notUsed);
}
Comment 1 Eric Gallager 2017-09-28 12:41:18 UTC
Confirmed that there's currently no warning.
Comment 2 Jonathan Wakely 2017-09-28 13:00:28 UTC
I'm not sure there *should* be a warning here.

I find -Wunused-parameter most useful to know when I can remove the name of the parameter, but you can't do that here (because it still needs a name to pass it to the recursive call).

It's rare that my reaction to a -Wunused-parameter warning is to change the function to remove the parameter entirely. YMMV.
Comment 3 Eric Gallager 2017-09-28 14:38:08 UTC
(In reply to Jonathan Wakely from comment #2)
> I'm not sure there *should* be a warning here.
> 
> I find -Wunused-parameter most useful to know when I can remove the name of
> the parameter, but you can't do that here (because it still needs a name to
> pass it to the recursive call).
> 
> It's rare that my reaction to a -Wunused-parameter warning is to change the
> function to remove the parameter entirely. YMMV.

There's also attribute unused and pragma unused, but yeah, it'd feel weird using either of those here, too... maybe this should be closed as WONTFIX instead...
Comment 4 Jonathan Wakely 2017-09-28 15:09:38 UTC
Those aren't portable, so aren't a good general solution.
Comment 5 Joshua T, Fisher 2017-09-28 17:51:36 UTC
(In reply to Jonathan Wakely from comment #2)
> It's rare that my reaction to a -Wunused-parameter warning is to change the
> function to remove the parameter entirely. YMMV.

Totally reasonable, I too rarely change the signature and use either attributes, removing the parameter name, or the void cast trick. However I have seen bugs pop up a couple times from important parameters being included, but not used, see:

https://github.com/nvpro-pipeline/VkHLF/commit/b6646c4773e8aef49c40e8684eca1382bf2e9d50

and my blog where I first encountered this in a closed source codebase:

http://www.nullterminatedstrings.com/c++/recursive-warning

Perhaps this doesn't qualify as a bug in the current -Wunused-parameter warning implementation, but would it be reasonable to add it as a separate warning?

Thanks for looking into this by the way folks!
Comment 6 Joshua T, Fisher 2017-09-28 21:00:02 UTC
(In reply to Joshua T, Fisher from comment #5)
> https://github.com/nvpro-pipeline/VkHLF/commit/b6646c4773e8aef49c40e8684eca1382bf2e9d50

Apologies this one doesn't apply. Just took another look and I just misremembered, this is just a normal -Wunused-parameter, I don't think they had it turned on.
Comment 7 Jonathan Wakely 2017-09-28 23:57:15 UTC
(In reply to Joshua T, Fisher from comment #5)
> and my blog where I first encountered this in a closed source codebase:
> 
> http://www.nullterminatedstrings.com/c++/recursive-warning

I'm a bit more persuaded having read this. If it points out you forgot to use it (for anything except the recursion) that could help find real bugs.
Comment 8 Joshua T, Fisher 2017-10-03 00:47:42 UTC
(In reply to Jonathan Wakely from comment #7)
> (In reply to Joshua T, Fisher from comment #5)
> > and my blog where I first encountered this in a closed source codebase:
> > 
> > http://www.nullterminatedstrings.com/c++/recursive-warning
> 
> I'm a bit more persuaded having read this. If it points out you forgot to
> use it (for anything except the recursion) that could help find real bugs.

That's what I was hoping! I feel like it's definitely something that could hit some real code bases. I find it unlikely I'm the only person ever to run into this.
Comment 9 Eric Gallager 2018-10-03 02:24:31 UTC
(In reply to Joshua T, Fisher from comment #8)
> (In reply to Jonathan Wakely from comment #7)
> > (In reply to Joshua T, Fisher from comment #5)
> > > and my blog where I first encountered this in a closed source codebase:
> > > 
> > > http://www.nullterminatedstrings.com/c++/recursive-warning
> > 
> > I'm a bit more persuaded having read this. If it points out you forgot to
> > use it (for anything except the recursion) that could help find real bugs.
> 
> That's what I was hoping! I feel like it's definitely something that could
> hit some real code bases. I find it unlikely I'm the only person ever to run
> into this.

Maybe it'd be possible to put it under a separate flag (e.g. -Wparameter-only-used-recursively) so that it'd be easier to focus on just that kind of warning individually? There are some codebases that turn off -Wunused-parameter specifically that still might want this warning.
Comment 10 Eric Gallager 2019-09-19 02:52:32 UTC
Martin Jambor's IPA-SRA rewrite might be relevant here; it sounded like the new IPA-SRA will remove parameters that are unused like this, but I didn't quite catch if it'd also warn about them...
Comment 11 Eric Gallager 2019-12-02 03:14:46 UTC
bug 32562 is related (that's for functions, this is for parameters)
Comment 12 Martin Jambor 2019-12-02 12:54:17 UTC
(In reply to Eric Gallager from comment #10)
> Martin Jambor's IPA-SRA rewrite might be relevant here; it sounded like the
> new IPA-SRA will remove parameters that are unused like this, but I didn't
> quite catch if it'd also warn about them...

IPA-SRA can remove parameters that are only used to be passed to
another function in a chain of recursive calls, it does not even have
to be a direct recursion (but it can).  It does not warn but it could.

One problem is however that IPA-SRA only modifies and analyzes "local"
functions, so in non-LTO mode it only works if all of the recursive
functions were static or in an anonymous C++ namespace.  With LTO,
anything not exported outside the result executable/shared object
would do but you'd only get your warnings at link-time.

Please also note that with this particular testcase, by the time IPA
passes see the function the recursion is converted to tail-call and
since IPA-CP (running just before IPA-SRA) can also remove totally
unused parameters, IPA-SRA does not do anything in this case even with
-fwhole-program.