Closed Bug 854105 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Networking: Cache, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(1 file, 2 obsolete files)

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++
This looks like a possible gcc bug :(
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)
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.
(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 :(
Michal, do you think you can take a look at this?
Flags: needinfo?(michal.novotny)
(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?
Flags: needinfo?(michal.novotny)
(In reply to Michal Novotny (:michal) from comment #6)
> So what would make gcc happy?

I think this would.
Attachment #735762 - Flags: review?(michal.novotny)
Assignee: nobody → mh+mozilla
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
Attachment #735762 - Flags: review?(michal.novotny) → review-
How about this?
Attachment #735769 - Flags: review?(michal.novotny)
Attachment #735762 - Attachment is obsolete: true
Attachment #735769 - Attachment is obsolete: true
Attachment #735769 - Flags: review?(michal.novotny)
Attachment #735771 - Flags: review?(michal.novotny) → review+
https://hg.mozilla.org/mozilla-central/rev/072adf9514eb
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.