RFR(xxs): 8167650: NMT should check for invalid MEMFLAGS.
Thomas Stüfe
thomas.stuefe at gmail.com
Wed Nov 23 19:56:38 UTC 2016
Thanks, Chris!
On Wed, 23 Nov 2016 at 18:45, Chris Plummer <chris.plummer at oracle.com>
wrote:
> On 11/23/16 5:47 AM, Thomas Stüfe wrote:
> > Hi David,
> >
> >
> >
> > On Wed, Nov 23, 2016 at 12:20 PM, David Holmes
> > <david.holmes at oracle.com <mailto:david.holmes at oracle.com>> wrote:
> >
> > Hi Thomas,
> >
> > On 23/11/2016 5:29 PM, Thomas Stüfe wrote:
> >
> > Hi David,
> >
> > ...
> >
> > My preference, obviously, is an assert inside flag_to_index, on
> > the value of index to be returned.
> >
> >
> > My latest webrev:
> >
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_MEMFLAGS/webrev.03/webrev/
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_MEMFLAGS/webrev.03/webrev/
> >
> >
> >
> > I did it the way you preferred, by checking the return value of
> > flag_to_index().
> Looks good to me. :)
>
> I'll run this through some internal testing and push it for you.
>
> Chris
> >
> > I did not expect this to become so prolonged, and I'm sorry about
> > that, but I really object to the callers of the function
> > validating its output.
> >
> >
> > No problem. I'm happy to have this issue closed.
> >
> > Kind Regards, Thomas
> >
> > Thanks,
> > David
> >
> > If we were in a decent language the bug would have been
> > impossible
> > to begin with.
> >
> > David
> > -----
> >
> >
> > Thanks, Thomas
> >
> >
> >
> > On 22/11/2016 1:21 AM, Thomas Stüfe wrote:
> >
> > Hi all,
> >
> > this small issue got fc extension, so lets wrap this
> > up. Thanks to
> > all for reviewing!
> >
> > New webrev:
> >
> >
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_MEMFLAGS/webrev.02/webrev/
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_MEMFLAGS/webrev.02/webrev/
> >
> >
> > <
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_MEMFLAGS/webrev.02/webrev/
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_MEMFLAGS/webrev.02/webrev/
> >>
> >
> > No changes to the last one but the updated copyrights
> > Chris did ask
> > for. Solution still follows Max' suggestion of
> > checking the index
> > right before consumption.
> >
> > Thanks, and Kind Regards, Thomas
> >
> >
> > On Mon, Nov 21, 2016 at 4:17 PM, Thomas Stüfe
> > <thomas.stuefe at gmail.com
> > <mailto:thomas.stuefe at gmail.com>
> > <mailto:thomas.stuefe at gmail.com <mailto:thomas.stuefe at gmail.com
> >>
> > <mailto:thomas.stuefe at gmail.com
> > <mailto:thomas.stuefe at gmail.com>
> > <mailto:thomas.stuefe at gmail.com
> > <mailto:thomas.stuefe at gmail.com>>>> wrote:
> >
> > Hi Chris,
> >
> > On Thu, Nov 17, 2016 at 8:54 PM, Chris Plummer
> > <chris.plummer at oracle.com
> > <mailto:chris.plummer at oracle.com>
> > <mailto:chris.plummer at oracle.com
> > <mailto:chris.plummer at oracle.com>>
> > <mailto:chris.plummer at oracle.com
> > <mailto:chris.plummer at oracle.com>
> > <mailto:chris.plummer at oracle.com
> > <mailto:chris.plummer at oracle.com>>>> wrote:
> >
> > On 10/18/16 11:49 PM, David Holmes wrote:
> >
> >
> >
> > On 19/10/2016 3:17 PM, Thomas Stüfe wrote:
> >
> >
> >
> > On Wed, Oct 19, 2016 at 3:21 AM, David Holmes
> > <david.holmes at oracle.com
> > <mailto:david.holmes at oracle.com>
> > <mailto:david.holmes at oracle.com <mailto:david.holmes at oracle.com
> >>
> > <mailto:david.holmes at oracle.com
> > <mailto:david.holmes at oracle.com>
> > <mailto:david.holmes at oracle.com <mailto:david.holmes at oracle.com
> >>>
> > <mailto:david.holmes at oracle.com
> > <mailto:david.holmes at oracle.com>
> > <mailto:david.holmes at oracle.com <mailto:david.holmes at oracle.com
> >>
> > <mailto:david.holmes at oracle.com
> > <mailto:david.holmes at oracle.com>
> > <mailto:david.holmes at oracle.com
> > <mailto:david.holmes at oracle.com>>>>>
> > wrote:
> >
> > On 18/10/2016 3:39 PM, Thomas Stüfe wrote:
> >
> > Hi David, Max,
> >
> > I changed the asserts according to Max' suggestion.
> > Instead of
> > checking inside flag_to_index, now I check before
> > callers of this
> > function use this value to access memory.
> >
> >
> > I don't see where Max suggested that??
> >
> >
> > Max wrote: " I think the decision on whether to access
> > a slot should
> > occur as close to memory accessing code as possible."
> > and proceeded
> > to suggest fixing VirtualMemorySnapshot::by_type() as
> > well.
> >
> >
> > I did not interpret that comment that way, and was
> > puzzled by the
> > reference to by_type.
> >
> >
> > It doesn't make sense to me to have all the callers of
> > flag_to_index
> > check what it returned instead of doing it inside
> > flag_to_index.
> >
> >
> > I disagree. Imho it makes sense to either check the
> > Memflags
> > enumeration input argument in flag_to_index() or the
> > returned index
> > before consumption. In both cases one knows the valid
> > value range.
> > Strictly speaking checking the index in
> > flag_to_index() cannot be
> > done because it is a faceless int type whose valid
> > values are
> > not yet
> > known.
> >
> >
> > The index has to fall in the range 0 <= index <=
> > mt_number_of_types,
> > and I was suggesting that it makes more sense to
> > verify this once in
> > flag_to_index() than in all the callers of flag_to_index.
> >
> > Hi Thomas,
> >
> > Just catching up on this thread. This is the same
> > conclusion I came
> > to. I don't understand what you mean by "it is a
> > faceless int type
> > whose valid values are not yet known".
> >
> >
> > Thank you for looking into this!
> >
> > "it is a faceless int type whose valid values are not
> > yet known" was
> > maybe expressed sloppily:
> >
> > My first patch checked the input enum "flag" argument
> > inside
> > NMTUtil::flag_to_index() for correct enum values :
> >
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_MEMFLAGS/webrev.00/webrev/
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_MEMFLAGS/webrev.00/webrev/
> >
> >
> > <
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_MEMFLAGS/webrev.00/webrev/
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_MEMFLAGS/webrev.00/webrev/
> >>
> >
> >
> >
> > <
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_MEMFLAGS/webrev.00/webrev/
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_MEMFLAGS/webrev.00/webrev/
> >
> >
> > <
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_MEMFLAGS/webrev.00/webrev/
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_MEMFLAGS/webrev.00/webrev/
> >>>
> >
> >
> > David did not like that, he thought it strange to
> > check an enum for
> > correct enum values. I can see his point. He would
> > have preferred
> > instead an assert inside NMTUtil::flag_to_index()
> > which before
> > returning checks the to-be-returned index for
> > 0>=i>=mt_number_of_types. I did not like that because
> > strictly
> > speaking inside of NMTUtil::flag_to_index() it is not
> > known that the
> > returned integer will be used by the caller as an
> > index into an
> > array
> > of mt_number_of_types length.
> >
> > It is all extreme nitpicking and in the end amounts to
> > the same :)
> >
> > The current patch does neither, but follows Max'
> > suggestion of
> > checking the index right before it is consumed to
> > access an array. I
> > think this is a good solution and very clear.
> >
> >
> >
> > BTW, I'll sponsor this fix for you once it is
> > finalized. Please
> > update the copyright dates first.
> >
> >
> > Thank you. I'll update the webrev and repost.
> >
> >
> >
> > thanks,
> >
> > Chris
> >
> >
> > Kind Regards, Thomas
> >
> >
> >
> > David
> >
> > It is all academical and mostly a matter of taste.
> >
> >
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_MEMFLAGS/webrev.01/webrev/index.html
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_MEMFLAGS/webrev.01/webrev/index.html
> >
> >
> > <
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_MEMFLAGS/webrev.01/webrev/index.html
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_MEMFLAGS/webrev.01/webrev/index.html
> >>
> >
> >
> >
> > <
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_MEMFLAGS/webrev.01/webrev/index.html
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_MEMFLAGS/webrev.01/webrev/index.html
> >
> >
> > <
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_MEMFLAGS/webrev.01/webrev/index.html
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_MEMFLAGS/webrev.01/webrev/index.html
> >>>
> >
> >
> > <
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_MEMFLAGS/webrev.01/webrev/index.html
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_MEMFLAGS/webrev.01/webrev/index.html
> >
> >
> > <
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_MEMFLAGS/webrev.01/webrev/index.html
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_MEMFLAGS/webrev.01/webrev/index.html
> >>
> >
> >
> >
> > <
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_MEMFLAGS/webrev.01/webrev/index.html
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_MEMFLAGS/webrev.01/webrev/index.html
> >
> >
> > <
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_MEMFLAGS/webrev.01/webrev/index.html
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_MEMFLAGS/webrev.01/webrev/index.html
> >>>>
> >
> >
> > As David correctly writes, this is technically not a
> > bug, so I guess
> > this will have to wait until java 10.
> >
> >
> > Yes, afraid so.
> >
> >
> > The fix is trivial and I will try to get fc extension
> > for this (now
> > that Goetz explained to me how to do this :). It seems
> > this is done
> > for many other non-bug issues as well.
> >
> > ..Thomas
> >
> >
> > Thanks, David
> >
> > Kind Regards, Thomas
> >
> >
> >
> > On Fri, Oct 14, 2016 at 12:57 AM, David Holmes
> > <david.holmes at oracle.com
> > <mailto:david.holmes at oracle.com>
> > <mailto:david.holmes at oracle.com <mailto:david.holmes at oracle.com
> >>
> > <mailto:david.holmes at oracle.com
> > <mailto:david.holmes at oracle.com>
> > <mailto:david.holmes at oracle.com <mailto:david.holmes at oracle.com
> >>>
> > <mailto:david.holmes at oracle.com
> > <mailto:david.holmes at oracle.com>
> > <mailto:david.holmes at oracle.com <mailto:david.holmes at oracle.com
> >>
> > <mailto:david.holmes at oracle.com
> > <mailto:david.holmes at oracle.com>
> > <mailto:david.holmes at oracle.com
> > <mailto:david.holmes at oracle.com>>>>
> > <mailto:david.holmes at oracle.com
> > <mailto:david.holmes at oracle.com>
> > <mailto:david.holmes at oracle.com <mailto:david.holmes at oracle.com
> >>
> > <mailto:david.holmes at oracle.com
> > <mailto:david.holmes at oracle.com>
> > <mailto:david.holmes at oracle.com <mailto:david.holmes at oracle.com
> >>>
> > <mailto:david.holmes at oracle.com
> > <mailto:david.holmes at oracle.com>
> > <mailto:david.holmes at oracle.com <mailto:david.holmes at oracle.com
> >>
> > <mailto:david.holmes at oracle.com
> > <mailto:david.holmes at oracle.com>
> > <mailto:david.holmes at oracle.com
> > <mailto:david.holmes at oracle.com>>>>>>
> > wrote:
> >
> > On 13/10/2016 10:53 PM, Thomas Stüfe wrote:
> >
> > Hi David,
> >
> > On Thu, Oct 13, 2016 at 12:08 PM, David Holmes
> > <david.holmes at oracle.com
> > <mailto:david.holmes at oracle.com>
> > <mailto:david.holmes at oracle.com <mailto:david.holmes at oracle.com
> >>
> > <mailto:david.holmes at oracle.com
> > <mailto:david.holmes at oracle.com>
> > <mailto:david.holmes at oracle.com <mailto:david.holmes at oracle.com
> >>>
> > <mailto:david.holmes at oracle.com
> > <mailto:david.holmes at oracle.com>
> > <mailto:david.holmes at oracle.com <mailto:david.holmes at oracle.com
> >>
> > <mailto:david.holmes at oracle.com
> > <mailto:david.holmes at oracle.com>
> > <mailto:david.holmes at oracle.com
> > <mailto:david.holmes at oracle.com>>>>
> > <mailto:david.holmes at oracle.com
> > <mailto:david.holmes at oracle.com>
> > <mailto:david.holmes at oracle.com <mailto:david.holmes at oracle.com
> >>
> > <mailto:david.holmes at oracle.com
> > <mailto:david.holmes at oracle.com>
> > <mailto:david.holmes at oracle.com <mailto:david.holmes at oracle.com
> >>>
> > <mailto:david.holmes at oracle.com
> > <mailto:david.holmes at oracle.com>
> > <mailto:david.holmes at oracle.com <mailto:david.holmes at oracle.com
> >>
> > <mailto:david.holmes at oracle.com
> > <mailto:david.holmes at oracle.com>
> > <mailto:david.holmes at oracle.com
> > <mailto:david.holmes at oracle.com>>>>>
> > <mailto:david.holmes at oracle.com
> > <mailto:david.holmes at oracle.com>
> > <mailto:david.holmes at oracle.com <mailto:david.holmes at oracle.com
> >>
> > <mailto:david.holmes at oracle.com
> > <mailto:david.holmes at oracle.com>
> > <mailto:david.holmes at oracle.com <mailto:david.holmes at oracle.com
> >>>
> > <mailto:david.holmes at oracle.com
> > <mailto:david.holmes at oracle.com>
> > <mailto:david.holmes at oracle.com <mailto:david.holmes at oracle.com
> >>
> > <mailto:david.holmes at oracle.com
> > <mailto:david.holmes at oracle.com>
> > <mailto:david.holmes at oracle.com
> > <mailto:david.holmes at oracle.com>>>>
> >
> > <mailto:david.holmes at oracle.com
> > <mailto:david.holmes at oracle.com>
> > <mailto:david.holmes at oracle.com <mailto:david.holmes at oracle.com
> >>
> > <mailto:david.holmes at oracle.com
> > <mailto:david.holmes at oracle.com>
> > <mailto:david.holmes at oracle.com <mailto:david.holmes at oracle.com
> >>>
> > <mailto:david.holmes at oracle.com
> > <mailto:david.holmes at oracle.com>
> > <mailto:david.holmes at oracle.com <mailto:david.holmes at oracle.com
> >>
> > <mailto:david.holmes at oracle.com
> > <mailto:david.holmes at oracle.com>
> > <mailto:david.holmes at oracle.com
> > <mailto:david.holmes at oracle.com>>>>>>>
> >
> > wrote:
> >
> > Hi Thomas,
> >
> > On 13/10/2016 3:49 PM, Thomas Stüfe wrote:
> >
> > Hi all,
> >
> > may I have plase a review for this tiny change? It
> > just adds some
> > assert to NMT.
> >
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8167650
> > <https://bugs.openjdk.java.net/browse/JDK-8167650>
> > <https://bugs.openjdk.java.net/browse/JDK-8167650
> > <https://bugs.openjdk.java.net/browse/JDK-8167650>>
> > <https://bugs.openjdk.java.net/browse/JDK-8167650
> > <https://bugs.openjdk.java.net/browse/JDK-8167650>
> > <https://bugs.openjdk.java.net/browse/JDK-8167650
> > <https://bugs.openjdk.java.net/browse/JDK-8167650>>>
> >
> > <https://bugs.openjdk.java.net/browse/JDK-8167650
> > <https://bugs.openjdk.java.net/browse/JDK-8167650>
> > <https://bugs.openjdk.java.net/browse/JDK-8167650
> > <https://bugs.openjdk.java.net/browse/JDK-8167650>>
> > <https://bugs.openjdk.java.net/browse/JDK-8167650
> > <https://bugs.openjdk.java.net/browse/JDK-8167650>
> > <https://bugs.openjdk.java.net/browse/JDK-8167650
> > <https://bugs.openjdk.java.net/browse/JDK-8167650>>>>
> > <https://bugs.openjdk.java.net/browse/JDK-8167650
> > <https://bugs.openjdk.java.net/browse/JDK-8167650>
> > <https://bugs.openjdk.java.net/browse/JDK-8167650
> > <https://bugs.openjdk.java.net/browse/JDK-8167650>>
> > <https://bugs.openjdk.java.net/browse/JDK-8167650
> > <https://bugs.openjdk.java.net/browse/JDK-8167650>
> > <https://bugs.openjdk.java.net/browse/JDK-8167650
> > <https://bugs.openjdk.java.net/browse/JDK-8167650>>>
> > <https://bugs.openjdk.java.net/browse/JDK-8167650
> > <https://bugs.openjdk.java.net/browse/JDK-8167650>
> > <https://bugs.openjdk.java.net/browse/JDK-8167650
> > <https://bugs.openjdk.java.net/browse/JDK-8167650>>
> > <https://bugs.openjdk.java.net/browse/JDK-8167650
> > <https://bugs.openjdk.java.net/browse/JDK-8167650>
> > <https://bugs.openjdk.java.net/browse/JDK-8167650
> > <https://bugs.openjdk.java.net/browse/JDK-8167650>>>>>
> >
> >
> > <https://bugs.openjdk.java.net/browse/JDK-8167650
> > <https://bugs.openjdk.java.net/browse/JDK-8167650>
> > <https://bugs.openjdk.java.net/browse/JDK-8167650
> > <https://bugs.openjdk.java.net/browse/JDK-8167650>>
> > <https://bugs.openjdk.java.net/browse/JDK-8167650
> > <https://bugs.openjdk.java.net/browse/JDK-8167650>
> > <https://bugs.openjdk.java.net/browse/JDK-8167650
> > <https://bugs.openjdk.java.net/browse/JDK-8167650>>>
> >
> > <https://bugs.openjdk.java.net/browse/JDK-8167650
> > <https://bugs.openjdk.java.net/browse/JDK-8167650>
> > <https://bugs.openjdk.java.net/browse/JDK-8167650
> > <https://bugs.openjdk.java.net/browse/JDK-8167650>>
> > <https://bugs.openjdk.java.net/browse/JDK-8167650
> > <https://bugs.openjdk.java.net/browse/JDK-8167650>
> > <https://bugs.openjdk.java.net/browse/JDK-8167650
> > <https://bugs.openjdk.java.net/browse/JDK-8167650>>>>
> > <https://bugs.openjdk.java.net/browse/JDK-8167650
> > <https://bugs.openjdk.java.net/browse/JDK-8167650>
> > <https://bugs.openjdk.java.net/browse/JDK-8167650
> > <https://bugs.openjdk.java.net/browse/JDK-8167650>>
> > <https://bugs.openjdk.java.net/browse/JDK-8167650
> > <https://bugs.openjdk.java.net/browse/JDK-8167650>
> > <https://bugs.openjdk.java.net/browse/JDK-8167650
> > <https://bugs.openjdk.java.net/browse/JDK-8167650>>>
> > <https://bugs.openjdk.java.net/browse/JDK-8167650
> > <https://bugs.openjdk.java.net/browse/JDK-8167650>
> > <https://bugs.openjdk.java.net/browse/JDK-8167650
> > <https://bugs.openjdk.java.net/browse/JDK-8167650>>
> > <https://bugs.openjdk.java.net/browse/JDK-8167650
> > <https://bugs.openjdk.java.net/browse/JDK-8167650>
> > <https://bugs.openjdk.java.net/browse/JDK-8167650
> > <https://bugs.openjdk.java.net/browse/JDK-8167650>>>>>> webrev:
> >
> >
> >
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>
> >
> > <
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>>
> >
> > <
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>
> >
> > <
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>>>
> >
> >
> >
> > <
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>
> > <
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>>
> >
> >
> > <
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>
> >
> > <
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>>>>
> >
> >
> >
> > <
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>
> >
> > <
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>>
> >
> >
> >
> > <
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>
> > <
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>>>
> >
> >
> > <
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>
> >
> > <
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>>
> >
> >
> >
> > <
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>
> > <
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_
> >>>>>
> >
> >
> >
> >
> > <
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>
> >
> > <
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>>
> >
> >
> >
> > <
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>
> > <
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>>>
> >
> >
> > <
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>
> >
> > <
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>>
> >
> >
> >
> > <
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>
> > <
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>>>>
> >
> >
> >
> > <
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>
> >
> > <
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>>
> >
> >
> >
> > <
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>
> > <
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>>>
> >
> >
> > <
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>
> >
> > <
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>>
> >
> >
> >
> > <
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_>
> > <
> http://cr.openjdk.java.net/~stuefe/webrevs/8167650-NMT-should-check_
> > <
> http://cr.openjdk.java.net/%7Estuefe/webrevs/8167650-NMT-should-check_
> >>>>>>
> >
> > MEMFLAGS/webrev.00/webrev/
> >
> > We had an ugly memory overwrite caused by this -
> > ultimately our
> > fault, because we fed an invalid memory flag to NMT -
> > but it was
> > difficult to find. An assert would have saved some time.
> >
> >
> > I'm a little perplexed with asserting that something
> > of MEMFLAGS
> > type
> > must be an actual MEMFLAGS value - it implies the
> > caller is coercing
> > plain int to MEMFLAGS, and I don't have much sympathy
> > if they mess
> > that up. Can't help wondering if there is some clever
> > C++ trick to
> > flag bad conversions at compile-time?
> >
> >
> > The error was caused by an uninitialized variable of
> > type MEMFLAGS.
> > This was our fault, we have heavily modified
> > allocation.hpp and
> > introduced an error then merging changes from
> > upstream. Due to a
> > merging error this lead to a case where Arena::_flags
> > was not
> > initialized and contained a very large value.
> >
> >
> > Ah I see. Lack of default initialization can be
> > annoying :)
> >
> > I admit it looks funny. If it bothers you, I could
> > instead check the
> > returned index to be in the range for the size of the
> > _malloc array
> > in MallocMemorySnapshot::by_type(). Technically, it
> > would mean the
> > same.
> >
> >
> > So I just realized that here:
> >
> > 62 // Map memory type to human readable name 63
> > static const
> > char* flag_to_name(MEMFLAGS flag) { 64 assert(flag >=
> > 0 && flag
> > < mt_number_of_types, "Invalid flag value %d.",
> > (int)flag); 65
> > return _memory_type_names[flag_to_index(flag)]; 66 }
> >
> > we call flag_to_index, so the assert is redundant as it
> is
> > already in
> > flag_to_index. Then presumably we change flag_to_index
> > to something
> > like this:
> >
> > static inline int flag_to_index(MEMFLAGS flag) { int
> > index = (flag &
> > 0xff); assert(index >= 0 && index <
> > mt_number_of_types, "Invalid
> > flag
> > value %d.", (int)flag); return index; }
> >
> > so we're validating the index rather than the flag.
> >
> > Cheers, David
> >
> >
> >
> >
> > The function that takes the index should validate the
> > index, so that
> > is fine.
> >
> > Which one were you actually passing the bad value to? :)
> >
> > This isn't a strong objection just musing if we can do
> > better.
> > And as
> > the hs repos are still closed, and likely to remain so
> > till early
> > next week, we have some slack time :)
> >
> >
> > :) Sure.
> >
> > Kind Regards, Thomas
> >
> >
> > Cheers, David
> >
> > Thank you!
> >
> > Thomas
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
>
>
>
More information about the hotspot-runtime-dev
mailing list