complain about re-declared functions with different modifiers

complain about re-declared functions with different modifiers

[Dan Carpenter]

I recently spent some time tracking down why Smatch wasn't parsing
nvme_put_ctrl() correctly.  It turned out the problem is that it's
declared as both inline and not inline so Smatch never parses it.

drivers/nvme/host/nvme.h
   472  static inline void nvme_get_ctrl(struct nvme_ctrl *ctrl)
   473  {
   474          get_device(ctrl->device);
   475  }
   476  
   477  static inline void nvme_put_ctrl(struct nvme_ctrl *ctrl)
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
It's an inline here.

   478  {
   479          put_device(ctrl->device);
   480  }
   481  
   482  static inline bool nvme_is_aen_req(u16 qid, __u16 command_id)
   483  {
   484          return !qid && command_id >= NVME_AQ_BLK_MQ_DEPTH;
   485  }
   486  
   487  void nvme_complete_rq(struct request *req);
   488  bool nvme_cancel_request(struct request *req, void *data, bool reserved);
   489  bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
   490                  enum nvme_ctrl_state new_state);
   491  bool nvme_wait_reset(struct nvme_ctrl *ctrl);
   492  int nvme_disable_ctrl(struct nvme_ctrl *ctrl);
   493  int nvme_enable_ctrl(struct nvme_ctrl *ctrl);
   494  int nvme_shutdown_ctrl(struct nvme_ctrl *ctrl);
   495  int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
   496                  const struct nvme_ctrl_ops *ops, unsigned long quirks);
   497  void nvme_uninit_ctrl(struct nvme_ctrl *ctrl);
   498  void nvme_start_ctrl(struct nvme_ctrl *ctrl);
   499  void nvme_stop_ctrl(struct nvme_ctrl *ctrl);
   500  void nvme_put_ctrl(struct nvme_ctrl *ctrl);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
But then it's re-declared as not inline.

   501  int nvme_init_identify(struct nvme_ctrl *ctrl);

Could Sparse print a warning for that?

regards,
dan carpenter

Re: complain about re-declared functions with different modifiers

[Linus Torvalds]

On Thu, May 14, 2020 at 7:05 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> But then it's re-declared as not inline.
>
> Could Sparse print a warning for that?

I actually think that would be a bug.

Having a non-inline declaration for an inline function sounds normal
and correct and even expected. I'd not be surprised at all if a
function is declared in a header file as non-inline (because there it
is), but then in the implementation it's defined as inline (because
later uses in that same file might want to inline it).

So warning about inline/non-inline differences is I think actively wrong.

HOWEVER.

In this case I wonder if the real difference is that first we have a
"static" definition of the symbol, and then later there's a non-static
declaration of it. That, to me, smells a bit odd - one has file scope,
the other has external scope, and particularly with the external scope
coming _after_ the file scope, which version of that same symbol does
a subsequent use then use?

Because a static symbol and a symbol with external linkage are clearly
not the same symbol. It's perfectly fine to have an external symbol
'x' that is shadowed by a static symbol 'x' in file scope.

But I wonder if smatch sees the *external* symbol nvme_put_ctrl()
(which doesn't have an inline definition!) rather than the static one
(which does!) because the external declaration comes after the static
definition.

So _that_ might be worth warning about: seeing an external declaration
after a static one makes for confusion.

I'm not sure that is the problem here.

                Linus

Re: complain about re-declared functions with different modifiers

[Luc Van Oostenryck]

On Thu, May 14, 2020 at 11:18:53AM -0700, Linus Torvalds wrote:
> On Thu, May 14, 2020 at 7:05 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > But then it's re-declared as not inline.
> >
> > Could Sparse print a warning for that?
> 
> I actually think that would be a bug.
> 
> Having a non-inline declaration for an inline function sounds normal
> and correct and even expected. I'd not be surprised at all if a
> function is declared in a header file as non-inline (because there it
> is), but then in the implementation it's defined as inline (because
> later uses in that same file might want to inline it).
> 
> So warning about inline/non-inline differences is I think actively wrong.
> 
> HOWEVER.
> 
> In this case I wonder if the real difference is that first we have a
> "static" definition of the symbol, and then later there's a non-static
> declaration of it. That, to me, smells a bit odd - one has file scope,
> the other has external scope, and particularly with the external scope
> coming _after_ the file scope, which version of that same symbol does
> a subsequent use then use?
> 
> Because a static symbol and a symbol with external linkage are clearly
> not the same symbol. It's perfectly fine to have an external symbol
> 'x' that is shadowed by a static symbol 'x' in file scope.
> 
> But I wonder if smatch sees the *external* symbol nvme_put_ctrl()
> (which doesn't have an inline definition!) rather than the static one
> (which does!) because the external declaration comes after the static
> definition.
> 
> So _that_ might be worth warning about: seeing an external declaration
> after a static one makes for confusion.
> 
> I'm not sure that is the problem here.

Not sure if it's related to Dan's problem or not but with the
following code:

	static inline int foo(void)
	{
		return 1;
	}
	
	extern int foo(void);
	
	int dummy(void)
	{
		return foo();
	}

the static definition of foo() and the extern declaration are
distinct symbols (in the sense that neither has its sym->same_symbol
pointing to the other). As far as I understand, this is correct
because they have a different 'scope'. The problem occurs later,
when doing the lookup in dummy(): which symbol should be returned?
It should be the static one but it's the extern one that is returned
(because it's the last one and symbols are added in stack order).
This can be easily with test-linearize showing that foo is not inlined.

It's not clear to me what can be done for this.

Also, as far as I understand the standard (6.9.2p2 ?), I think
that if the 'extern' keyword would not have been used (like in Dan's
example), then the second, non-inline, occurence of 'foo' should
refer to the first, inline, one and thus be the same symbol.
This is currently the case since last November when a non-static
non-extern definition follows a static declaration but not when
the declaration follows the definition.

-- Luc

Re: complain about re-declared functions with different modifiers

[Linus Torvalds]

On Thu, May 14, 2020 at 1:56 PM Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
>
> the static definition of foo() and the extern declaration are
> distinct symbols (in the sense that neither has its sym->same_symbol
> pointing to the other). As far as I understand, this is correct
> because they have a different 'scope'.

So it looks like gcc disagrees.

Gcc thinks they are the same symbol. It allows you to re-define it as
static vs external, but it needs to be defined to the same thing.

So yes, "extern" and "static" have different external visibility, but
the scope of the symbol is the same (file scope), and it's the same
symbol.

I didn't check the standard for how it's supposed to work, but I wrote
this silly test program:


    static int __attribute__((noinline)) external(void) { return 0; }
    static int __attribute__((noinline)) internal(void) { return 1; }

    extern int __attribute__((alias("external"))) a(void);
    static int __attribute__((alias("internal"))) a(void);

    int main(int argc, char **argv)
    {
        return a();
    }

to see which one gcc would pick - if it considered them separate symbols.

And gcc refuses to compile it with

   error: redefinition of ‘a’

which is admittedly very sane.

So I think sparse is in the wrong here, and we should consider both
external and static symbols to be in the same scope and conflict with
each other unless their declarations match.

                 Linus

Re: complain about re-declared functions with different modifiers

[Dan Carpenter]

On Thu, May 14, 2020 at 10:56:04PM +0200, Luc Van Oostenryck wrote:
> Not sure if it's related to Dan's problem or not but with the
> following code:
> 
> 	static inline int foo(void)
> 	{
> 		return 1;
> 	}
> 	
> 	extern int foo(void);
> 	
> 	int dummy(void)
> 	{
> 		return foo();
> 	}
> 
> the static definition of foo() and the extern declaration are
> distinct symbols (in the sense that neither has its sym->same_symbol
> pointing to the other). As far as I understand, this is correct
> because they have a different 'scope'. The problem occurs later,
> when doing the lookup in dummy(): which symbol should be returned?

Yeah.  That's it.  When I see the call, I want to parse the statements
so I need the symbol with the implementation.

regards,
dan carpenter

Re: complain about re-declared functions with different modifiers

[Luc Van Oostenryck]

On Fri, May 15, 2020 at 04:36:17PM +0300, Dan Carpenter wrote:
> Yeah.  That's it.  When I see the call, I want to parse the statements
> so I need the symbol with the implementation.

OK, thanks. I'll look at this this WE. 

-- Luc

Re: complain about re-declared functions with different modifiers

[Luc Van Oostenryck]

On Thu, May 14, 2020 at 03:32:38PM -0700, Linus Torvalds wrote:
> 
> And gcc refuses to compile it with
> 
>    error: redefinition of ‘a’
> 
> which is admittedly very sane.
> 
> So I think sparse is in the wrong here, and we should consider both
> external and static symbols to be in the same scope and conflict with
> each other unless their declarations match.

Yes, I agree. I'll see what can be done for this.

-- Luc

Re: complain about re-declared functions with different modifiers

[Derek M Jones]

Linus,

> So I think sparse is in the wrong here, and we should consider both
> external and static symbols to be in the same scope and conflict with
> each other unless their declarations match.

Yes.

The C standard only has one one file scope (i.e., the is no
file scope+extra_stuff):
http://c0x.coding-guidelines.com/6.2.1.html

Difference between extern/static revolve around the concept of linkage:
http://c0x.coding-guidelines.com/6.2.2.html

-- 
Derek M. Jones           Evidence-based software engineering
tel: +44 (0)1252 520667  blog:shape-of-code.coding-guidelines.com

Re: complain about re-declared functions with different modifiers

[Luc Van Oostenryck]

On Fri, May 15, 2020 at 04:36:17PM +0300, Dan Carpenter wrote:
> On Thu, May 14, 2020 at 10:56:04PM +0200, Luc Van Oostenryck wrote:
> > Not sure if it's related to Dan's problem or not but with the
> > following code:
> > 
> > 	static inline int foo(void)
> > 	{
> > 		return 1;
> > 	}
> > 	
> > 	extern int foo(void);
> > 	
> > 	int dummy(void)
> > 	{
> > 		return foo();
> > 	}
> > 
> > the static definition of foo() and the extern declaration are
> > distinct symbols (in the sense that neither has its sym->same_symbol
> > pointing to the other). As far as I understand, this is correct
> > because they have a different 'scope'. The problem occurs later,
> > when doing the lookup in dummy(): which symbol should be returned?
> 
> Yeah.  That's it.  When I see the call, I want to parse the statements
> so I need the symbol with the implementation.

There must something else too.
In the example here above I added 'extern' to the second declaration.
But in your first example no storage was given:
	void nvme_put_ctrl(struct nvme_ctrl *ctrl);'
and in this case, Sparse give it the storage/linkage from the previous
declaration which was 'static'.
So in the case, the second occurent has its ->same_symbol set to the
previous static inline version and it's ->definition points to it too.

So, I think everything is correct here regarding Sparse (the question
of a warning is something else: IMO none should be give for a static
declaration/definition followed by a plain declaration (thus implicitly
static) but well if followed by an extern one. One is also when
a static follow an extern or a plain (implicitly extern).

Doesn't smatch uses ->same_symbol and more importantly ->definition?


Regards,
-- Luc

Re: complain about re-declared functions with different modifiers

[Dan Carpenter]

On Sun, May 17, 2020 at 06:56:37AM +0200, Luc Van Oostenryck wrote:
> On Fri, May 15, 2020 at 04:36:17PM +0300, Dan Carpenter wrote:
> > On Thu, May 14, 2020 at 10:56:04PM +0200, Luc Van Oostenryck wrote:
> > > Not sure if it's related to Dan's problem or not but with the
> > > following code:
> > > 
> > > 	static inline int foo(void)
> > > 	{
> > > 		return 1;
> > > 	}
> > > 	
> > > 	extern int foo(void);
> > > 	
> > > 	int dummy(void)
> > > 	{
> > > 		return foo();
> > > 	}
> > > 
> > > the static definition of foo() and the extern declaration are
> > > distinct symbols (in the sense that neither has its sym->same_symbol
> > > pointing to the other). As far as I understand, this is correct
> > > because they have a different 'scope'. The problem occurs later,
> > > when doing the lookup in dummy(): which symbol should be returned?
> > 
> > Yeah.  That's it.  When I see the call, I want to parse the statements
> > so I need the symbol with the implementation.
> 
> There must something else too.
> In the example here above I added 'extern' to the second declaration.
> But in your first example no storage was given:
> 	void nvme_put_ctrl(struct nvme_ctrl *ctrl);'
> and in this case, Sparse give it the storage/linkage from the previous
> declaration which was 'static'.
> So in the case, the second occurent has its ->same_symbol set to the
> previous static inline version and it's ->definition points to it too.
> 
> So, I think everything is correct here regarding Sparse (the question
> of a warning is something else: IMO none should be give for a static
> declaration/definition followed by a plain declaration (thus implicitly
> static) but well if followed by an extern one. One is also when
> a static follow an extern or a plain (implicitly extern).
> 
> Doesn't smatch uses ->same_symbol and more importantly ->definition?

Ah...  No I wasn't.

I also need to merge with the last Sparse.  The last time I merged was
in September and back then the ->scope pointers were different so the
two functions weren't counted as the ->same_symbol.

It should all work now.  Thanks!

regards,
dan carpenter