854105 - nsDiskCacheMap.cpp:1029:68: error: array subscript is above array bounds [-Werror=array-bounds] with gcc 4.7 PGO on linux64

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Mike Hommey [:glandium]]

https://tbpl.mozilla.org/php/getParsedLog.php?id=21015138&tree=Try

GCC 4.7 PGO can be tested on try with the following:

--- a/build/unix/mozconfig.linux
+++ b/build/unix/mozconfig.linux
@@ -1,4 +1,4 @@
 . "$topsrcdir/build/mozconfig.common"
-
-CC=/tools/gcc-4.5-0moz3/bin/gcc
-CXX=/tools/gcc-4.5-0moz3/bin/g++
+mk_add_options MOZ_PGO=1
+CC=/tools/gcc-4.7.2-0moz1/bin/gcc
+CXX=/tools/gcc-4.7.2-0moz1/bin/g++

Comment 1

[Mike Hommey [:glandium]]

This looks like a possible gcc bug :(

Comment 2

[Mike Hommey [:glandium]]

Filed bug http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56698

But we do have a problem in https://hg.mozilla.org/mozilla-central/file/bcf09432affd/netwerk/cache/nsDiskCacheMap.cpp#l1028 when fileIndex == 0, which can happen, if size is > 4 * BLOCK_SIZE_FOR_INDEX(3) (16kB)

Comment 3

[Mike Hommey [:glandium]]

Interestingly, if I modify CalculateFileIndex to return 3 instead of 0, the error doesn't happen. So it might be a case of confusing gcc warning/error message.

Comment 4

[Trevor Saunders (:tbsaunde)]

(In reply to Mike Hommey [:glandium] from comment #3)
> Interestingly, if I modify CalculateFileIndex to return 3 instead of 0, the
> error doesn't happen. So it might be a case of confusing gcc warning/error
> message.

fileIndex is a uint32_t so 0 - 1 is UINT32_MAX right?  in which case this warning isn't even that confusing other than the sucky part of unsigned types.

In any case this use of CalculateFileIndex()certainly seems busted, but I have no idea how this should actually work :(

Comment 5

[Honza Bambas (:mayhemer)]

Michal, do you think you can take a look at this?

Comment 6

[Michal Novotny [:michal]]

(In reply to Mike Hommey [:glandium] from comment #2)
> But we do have a problem in
> https://hg.mozilla.org/mozilla-central/file/bcf09432affd/netwerk/cache/
> nsDiskCacheMap.cpp#l1028 when fileIndex == 0, which can happen, if size is >
> 4 * BLOCK_SIZE_FOR_INDEX(3) (16kB)

We don't have a problem because we call nsDiskCacheMap::WriteDataCacheBlocks() from nsDiskCacheStreamIO::CloseOutputStream() where we allocate buffer only up to kMaxBufferSize which is 16kB. I.e. we know for sure that CalculateFileIndex() returns 1-3 here. So what would make gcc happy?

Comment 7

[Mike Hommey [:glandium]]

Attachment: Avoid nsDiskCacheMap::CalculateFileIndex returning 0

(In reply to Michal Novotny (:michal) from comment #6)
> So what would make gcc happy?

I think this would.

Comment 8

[Michal Novotny [:michal]]

Comment on attachment 735762 [details] [diff] [review]
Avoid nsDiskCacheMap::CalculateFileIndex returning 0

(In reply to Mike Hommey [:glandium] from comment #7)
> (In reply to Michal Novotny (:michal) from comment #6)
> > So what would make gcc happy?
> 
> I think this would.

But CalculateFileIndex() is supposed to return 0-3 here http://hg.mozilla.org/mozilla-central/annotate/9db46ddfb517/netwerk/cache/nsDiskCacheMap.cpp#l904

0 means separate file
1,2,3 mean blockfile 0,1,2

Comment 9

[Mike Hommey [:glandium]]

Attachment: Avoid nsDiskCacheMap::CalculateFileIndex returning 0

How about this?

Comment 10

[Mike Hommey [:glandium]]

Attachment: Avoid gcc 4.7 complaining about array out of bounds in nsDiskCacheMap::WriteDataCacheBlocks

Better version.

Comment 11

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/072adf9514eb

Comment 12

[Ryan VanderMeulen (PTO, back 6-April)]

https://hg.mozilla.org/mozilla-central/rev/072adf9514eb