Bogus 'not declared' warning

Bogus 'not declared' warning

[Matthew Wilcox]

Seems to me that sparse ignores 'static' forward declarations, leading
to false warnings like this:

/home/willy/kernel/linux-2.6/drivers/scsi/sym53c8xx_2/sym_hipd.c:3786:5: warning: symbol 'sym_compute_residual' was not declared. Should it be static?

$ grep -n sym_compute_residual drivers/scsi/sym53c8xx_2/*
drivers/scsi/sym53c8xx_2/sym_hipd.c:61:static int sym_compute_residual(struct sym_hcb *np, struct sym_ccb *cp);
drivers/scsi/sym53c8xx_2/sym_hipd.c:3033:               cp->sv_resid = sym_compute_residual(np, cp);
drivers/scsi/sym53c8xx_2/sym_hipd.c:3786:int sym_compute_residual(struct sym_hcb *np, struct sym_ccb *cp)

This is a self-built sparse from commit
fa7d792f66cf02b86dd834d06b267ec66ddfe700.  The two commits since don't
appear relevant.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: Bogus 'not declared' warning

[Josh Triplett]

[-- Attachment #1: Type: text/plain, Size: 1857 bytes --]

Matthew Wilcox wrote:
> Seems to me that sparse ignores 'static' forward declarations, leading
> to false warnings like this:
> 
> /home/willy/kernel/linux-2.6/drivers/scsi/sym53c8xx_2/sym_hipd.c:3786:5: warning: symbol 'sym_compute_residual' was not declared. Should it be static?
> 
> $ grep -n sym_compute_residual drivers/scsi/sym53c8xx_2/*
> drivers/scsi/sym53c8xx_2/sym_hipd.c:61:static int sym_compute_residual(struct sym_hcb *np, struct sym_ccb *cp);
> drivers/scsi/sym53c8xx_2/sym_hipd.c:3033:               cp->sv_resid = sym_compute_residual(np, cp);
> drivers/scsi/sym53c8xx_2/sym_hipd.c:3786:int sym_compute_residual(struct sym_hcb *np, struct sym_ccb *cp)

Interesting; yes, it looks like the routine emitting that warning
doesn't check for a symbol marked static by having a previous static
declaration.  That warning comes from check_duplicates in evaluate.c.
Seeing it means Sparse didn't see any previous declaration of the
symbol by checking the same_symbol linked list, which seems wrong.
check_declaration in symbol.c hooks symbols into those lists.  That
implies 1) the scopes don't match and 2) one or the other symbol
doesn't have extern.  The latter should clearly hold true (that case,
IIRC, handles letting you put extern declarations inside inner
scopes).  I think the former occurs because one declaration has file
scope and the other one global scope, but that has a chicken-and-egg
issue in the case of a static forward declaration: the static forward
declaration applies, and makes the later declaration have file scope.

I *think* the right fix involves changing check_declaration to check
for this case specifically:
if (next->scope == file_scope && sym->scope == global_scope) {
	sym->scope = file_scope;
	sym->same_symbol = next;
	return;
}

Will test this later.

- Josh Triplett



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: Bogus 'not declared' warning

[Josh Triplett]

[-- Attachment #1: Type: text/plain, Size: 2360 bytes --]

Josh Triplett wrote:
> Matthew Wilcox wrote:
>> Seems to me that sparse ignores 'static' forward declarations, leading
>> to false warnings like this:
>>
>> /home/willy/kernel/linux-2.6/drivers/scsi/sym53c8xx_2/sym_hipd.c:3786:5: warning: symbol 'sym_compute_residual' was not declared. Should it be static?
>>
>> $ grep -n sym_compute_residual drivers/scsi/sym53c8xx_2/*
>> drivers/scsi/sym53c8xx_2/sym_hipd.c:61:static int sym_compute_residual(struct sym_hcb *np, struct sym_ccb *cp);
>> drivers/scsi/sym53c8xx_2/sym_hipd.c:3033:               cp->sv_resid = sym_compute_residual(np, cp);
>> drivers/scsi/sym53c8xx_2/sym_hipd.c:3786:int sym_compute_residual(struct sym_hcb *np, struct sym_ccb *cp)
> 
> Interesting; yes, it looks like the routine emitting that warning
> doesn't check for a symbol marked static by having a previous static
> declaration.  That warning comes from check_duplicates in evaluate.c.
> Seeing it means Sparse didn't see any previous declaration of the
> symbol by checking the same_symbol linked list, which seems wrong.
> check_declaration in symbol.c hooks symbols into those lists.  That
> implies 1) the scopes don't match and 2) one or the other symbol
> doesn't have extern.  The latter should clearly hold true (that case,
> IIRC, handles letting you put extern declarations inside inner
> scopes).  I think the former occurs because one declaration has file
> scope and the other one global scope, but that has a chicken-and-egg
> issue in the case of a static forward declaration: the static forward
> declaration applies, and makes the later declaration have file scope.
> 
> I *think* the right fix involves changing check_declaration to check
> for this case specifically:
> if (next->scope == file_scope && sym->scope == global_scope) {
> 	sym->scope = file_scope;
> 	sym->same_symbol = next;
> 	return;
> }

Turns out this will take a bit more work to fix.  check_declaration
seems like the wrong place for this check, and in any case the code
above won't properly hook the symbol into its scope since it doesn't
call bind_symbol.  bind_symbol in turn has some dead code and other
problems.  I don't plan to address this for the 0.4.1 release,
other than by adding a test case for this, because I don't want to
further delay the other bugfixes in 0.4.1.

- Josh Triplett


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]