664257 - look at clang static analyzer output and flag any real bugs

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Virtual Machine, defect, P3)

Description

[Tommy Reilly]

I find 46 errors and they look mostly harmless when I do the "scan-build xcodebuild" thing.   Mostly dead code issues but 16 could be real issues.

Comment 1

[Tommy Reilly]

Attachment: necessary patch to make scan-build work

Comment 2

[Tommy Reilly]

Attachment: results

Comment 3

[Tommy Reilly]

results are from running (in platform/mac/avmshell directory)

$ scan-build xcodebuild 

scan-build is from http://clang-analyzer.llvm.org/ checker-257.tar.bz2

Comment 4

[Tommy Reilly]

Attachment: Fix bits in page possible NULL analyzer warning

Comment 5

[Tommy Reilly]

Attachment: Fix currentTotal dead init

Comment 6

[Tommy Reilly]

Attachment: ifdef DEBUG GetUserPointer call

Comment 7

[Tommy Reilly]

Attachment: Remove switch

Comment 8

[Tommy Reilly]

These patches fix all the MMgc issues found by the analyzer

Comment 9

[Tommy Reilly]

This almost works with xp build but it appears to be analyzing system headers and barfing profusely:

$ LDPLUSPLUS=~/llvm-2.9/checker-257/libexec/c++-analyzer CCC_CC=gcc-4.2 CCC_CXX=g++-4.2 CLANG=~/llvm-2.9/checker-257/bin/clang CLANG_CXX=~/llvm-2.9/checker-257/bin/clang++ CXX=~/llvm-2.9/checker-257/libexec/c++-analyzer CC=~/llvm-2.9/checker-257/libexec/ccc-analyzer make

Comment 10

[Lars T Hansen]

Comment on attachment 542559 [details] [diff] [review]
Fix currentTotal dead init

If startingTotal isn't the proper init then 0 should be - missing inits are problems far more often than redundant inits.

Comment 11

[Lars T Hansen]

Comment on attachment 542557 [details] [diff] [review]
Fix bits in page possible NULL analyzer warning

OK i guess, though making m_bitsInPage const would be the better fix.

Comment 12

[Lars T Hansen]

Comment on attachment 542560 [details] [diff] [review]
ifdef DEBUG GetUserPointer call

The tag for the patch says DEBUG, the patch says MMGC_MEMORY_INFO.  Which is it?  Also, if it is DEBUG then the patch is pointless, GetUserPointer is a no-op in release builds so that we can avoid these kinds of #ifdefs.

Comment 13

[Lars T Hansen]

Comment on attachment 542561 [details] [diff] [review]
Remove switch

What's the justification here?  The code is clearly correct as is, and may even be faster in the common case (though nobody cares about that).

Comment 14

[Tommy Reilly]

(In reply to comment #13)
> Comment on attachment 542561 [details] [diff] [review] [review]
> Remove switch
> 
> What's the justification here?  The code is clearly correct as is, and may
> even be faster in the common case (though nobody cares about that).

The static analyzer isn't smart enough to realize that the switch value is in the set [0-4] so it thinks allocs could be NULL.

Comment 15

[Tommy Reilly]

(In reply to comment #12)
> The tag for the patch says DEBUG, the patch says MMGC_MEMORY_INFO.  Which is
> it?  Also, if it is DEBUG then the patch is pointless, GetUserPointer is a
> no-op in release builds so that we can avoid these kinds of #ifdefs.

Agreed, I'll see if I can come up with a solution that keeps us ifdef clean.

Comment 16

[Tommy Reilly]

Attachment: Better version that addresses Lar's feedback

Comment 17

[Tommy Reilly]

(In reply to comment #11)
> Comment on attachment 542557 [details] [diff] [review] [review]
> Fix bits in page possible NULL analyzer warning
> 
> OK i guess, though making m_bitsInPage const would be the better fix.

m_bitsInPage is const!   So this is another case where the static analyzer just isn't smart enough.

Comment 18

[Tommy Reilly]

Attachment: Fix the bitsInPage issue with cleaner code

Comment 19

[Lars T Hansen]

Comment on attachment 542792 [details] [diff] [review]
Fix the bitsInPage issue with cleaner code

This patch appears to be identical to the previous one.  Did you upload the wrong one?

Comment 20

[Lars T Hansen]

Comment on attachment 542561 [details] [diff] [review]
Remove switch

I will not, nor do I think that we in general should, rewrite the code just because the analyzer is too dumb to figure out what the code does, so long as it is clear to a human reader what is going on.

Comment 21

[Tommy Reilly]

Attachment: New bitsInPage patch, forgot to qref last time.

Comment 22

[Tamarin Bot]

changeset: 6436:97c56359f161
user:      Tommy Reilly <[e-mail]>
summary:   Bug 664257 - Clean up dead initialization found by clang analyzer (r=lhansen)

http://hg.mozilla.org/tamarin-redux/rev/97c56359f161

Comment 23

[Lars T Hansen]

Comment on attachment 542795 [details] [diff] [review]
New bitsInPage patch, forgot to qref last time.

I'm not sure about this one.  I seem to remember there was a good reason (probably related to OOM) why the bits alloc came before the block alloc.  I have to think about it.  Maybe it is that if the bits alloc fails and we shut down due to OOM the allocated block will be orphaned if it is not owned by the collector; if it is owned by the collector it may be reaped because it is empty.

Comment 24

[Lars T Hansen]

(In reply to comment #23)
> ... if it is owned by the collector it may be reaped because
> it is empty.

And then, if we /don't/ shut down we have an inconsistent heap.

Comment 25

[Tommy Reilly]

(In reply to comment #24)
> (In reply to comment #23)
> > ... if it is owned by the collector it may be reaped because
> > it is empty.
> 
> And then, if we /don't/ shut down we have an inconsistent heap.

So right now the bits are still allocated before the block is linked in so in the case of bits oom then the block is just orphaned (which is fine because the shutdown sequence will return the memory to the OS anyways).

Comment 26

[Tommy Reilly]

(In reply to comment #20)
> I will not, nor do I think that we in general should, rewrite the code just
> because the analyzer is too dumb to figure out what the code does, so long
> as it is clear to a human reader what is going on.

Yeah in this case I think readability is a wash so I don't feel strongly about it, its not like the patch makes it harder or easier to read.   

Tangential q: why isn't the bibopAllocFloat allocator in this list?  It appears to have a quick list from a cursory reading.

Comment 27

[Lars T Hansen]

> Tangential q: why isn't the bibopAllocFloat allocator in this list?  It
> appears to have a quick list from a cursory reading.

That's probably an oversight.  I could wave my hands and say something about how that list will usually be short and there's no explicit or RC freeing onto it so it'll stay short blabla but it wouldn't be completely convincing.

I'll file a bug.  Thanks.

Comment 28

[Lars T Hansen]

Comment on attachment 542795 [details] [diff] [review]
New bitsInPage patch, forgot to qref last time.

Trying to work through what would happen here.

We allocate a block for the allocator and that succeeds.  GC::AllocBlock marks the block as kGCAllocPage.  But the block header is still uninitialized.

Then we initialize the first few fields, and we get to the point where we need to initialize b->bits.  Suppose the free list is empty so we call heapAlloc().  Suppose GCHeap::AllocHelper fails, so we call SendFreeMemorySignal.  This results in GC::memoryStatusChange being invoked, and we're OnThread, so we Collect().  The collector performs a conservative stack scan and scans some objects conservatively; assume the stack or one such object contains a bit pattern that looks like a pointer into the freshly allocated block.  This would happen in GC::TraceConservativePointer.  Remember, that page is marked kGCAllocPage, so that's the first big block in TraceConservativePointer.  We get to this:

    itemNum = GCAlloc::GetObjectIndex(block, item);

which will fail because GetObjectIndex will require block->items to be initialized but it was not initialized yet in GCAlloc.  That's fixable, so assume we fix it.  We then get to the main test:

    gcbits_t& bits2 = block->bits[GCAlloc::GetBitsIndex(block, item)];

block->bits is NULL and this is not fixable in GCAlloc because we're in the process of allocating it.

Am I missing something?  ISTR that the reason there is a comment in GCAlloc, "Get bitmap space; this may trigger OOM handling.", is precisely because of this possibility.

We could fix this by not marking the page as a GC page in GC::AllocBlock until we've allocated the bits space (and that would arguably be cleanest in any case), but that's somewhat more radical surgery, and all for the purpose of pacifying Clang.

Tentative R- then.

Comment 29

[Felix S. Klock II [:pnkfelix, :fklock]]

(I cannot trivially tell how much of this task remains uncompleted.  We could probably safely close this ticket and reopen it later if someone else decided we should give clang another whirl.)

Comment 30

[Sylvestre Ledru [:Sylvestre]]

Tamarin is a dead project now. Mass WONTFIX.

Comment 31

[Sylvestre Ledru [:Sylvestre]]

Tamarin isn't maintained anymore. WONTFIX remaining bugs.