PATCH: using mixed types in MIN2/MAX2 functions

[Dan Horák]

On Fri, 13 Jun 2014 14:10:53 +0200
Bengt Rutisson <bengt.rutisson at oracle.com> wrote:

> 
> Hi Dan,
> 
> I haven't looked in detail at this fix, but I have a question. How do
> we make sure that we don't introduce new calls to MAX2() that mix
> size_t and uintx? It seems like this fix is a bit fragile in that
> sense.

Good question and maybe doesn't have an answer. The compiler probably
can't distinguish between size_t and uintx when they are both
"unsigned int" effectively, so it's only s390 (32-bit) I know about
where the problem can be made visible (size_t is "unsigned long").
Ideally someone with a deep knowledge could go through the code and
decide whether the arguments and attributes are using the correct types.

> What do you think about introducing specialized MAX2() definitions
> for the size_t and uintx combinations. That would avoid the casting
> in the code and will be safer going forward.

I think it can't work when both size_t and uintx are "unsigned int". The
newly introduced functions would conflict with the uintx/uintx variant,
so they would have to made specific for case where size_t is not
"unsigned int".

> One detail also. Why does *hum_bytes have to be cast to size_t here?
> It is already a size_t*.
> http://fedora.danny.cz/openjdk/size_t-1/webrev/src/share/vm/gc_implementation/g1/concurrentMark.cpp.udiff.html

ah, it's likely a result from applying the original patch based on the
dev repository onto the hs-gc repository and fixing only newly appeared
problems and skipping a review of the old changes

> Then a formality comment. I think you have to post the webrev on 
> cr.openjdk.java.net to get the legal stuff correct.

hm, I've used this procedure (my webrev, then someone privileged takes
it and pushes it) few times in the past and it worked.


		Dan


> Thanks,
> Bengt
> 
> 
> On 2014-06-13 09:55, Dan Horák wrote:
> > On Thu, 12 Jun 2014 21:49:40 +0200
> > Jesper Wilhelmsson <jesper.wilhelmsson at oracle.com> wrote:
> >
> >> Hi,
> >>
> >> Which repository is the patch built on? Since it is mostly GC code
> >> it would be good if it goes in through jdk9/hs-gc.
> >>
> >> The changes looks good in the webrev, but the patch did not apply
> >> to a recent jdk9/hs-gc. Especially the collector policy code has
> >> changed a lot recently.
> >>
> >> I would prefer to look at the changes applied in my workspace
> >> before approving it, so if you could update to a recent hs-gc it
> >> would be great.
> > I have refreshed the patch for the hs-gc repository, please see
> > http://fedora.danny.cz/openjdk/size_t-1/webrev/
> >
> >
> > 	Thanks,
> >
> > 		Dan
> >   
> >> Thanks!
> >> /Jesper
> >>
> >> Vladimir Kozlov skrev 12/6/14 20:57:
> >>> Thank you, Dan
> >>>
> >>> I think casting is acceptable solution. Someone in GC group should
> >>> review and sponsor this since you touched GC code mostly.
> >>>
> >>> Regards,
> >>> Vladimir
> >>>
> >>> On 6/12/14 2:05 AM, Dan Horák wrote:
> >>>> Hello,
> >>>>
> >>>> this the last and largest part of my changes required on s390
> >>>> (32-bit) to build OpenJDK out of the box. The MIN2/MAX2 functions
> >>>> are implemented using templates and require both arguments to be
> >>>> of the same type. This is a problem when one parameter is of
> >>>> size_t type and the second of uintx type and the platform has
> >>>> size_t defined as eg. unsigned long as on s390 (32-bit).
> >>>>
> >>>> As a solution [1] I chose to typecast the smaller type (uintx) to
> >>>> size_t which is of same or larger size.
> >>>>
> >>>> [1] http://fedora.danny.cz/openjdk/size_t/webrev/
> >>>>
> >>>> Note: I'm Red Hat employee (dhorak at redhat.com), so I should be
> >>>> covered by Red Hat's CLA for my contributions.
> >>>>
> >>>>
> >>>>      With regards
> >>>>
> >>>>          Dan
> >>>>
>