* [libdrm][PATCH 1/2] random: Use unsigned long for seed @ 2015-02-09 21:39 Jan Vesely 2015-02-09 21:39 ` [libdrm][PATCH 2/2] Fix gcc -Wextra warnings Jan Vesely ` (2 more replies) 0 siblings, 3 replies; 16+ messages in thread From: Jan Vesely @ 2015-02-09 21:39 UTC (permalink / raw) To: dri-devel This is more consistent with the rest, and avoids potential undefined behavior (signed overflow) in drmRandom() Signed-off-by: Jan Vesely <jan.vesely@rutgers.edu> --- xf86drmRandom.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/xf86drmRandom.c b/xf86drmRandom.c index ecab9e2..a084b86 100644 --- a/xf86drmRandom.c +++ b/xf86drmRandom.c @@ -98,7 +98,7 @@ typedef struct RandomState { unsigned long q; /* m div a */ unsigned long r; /* m mod a */ unsigned long check; - long seed; + unsigned long seed; } RandomState; #if RANDOM_MAIN @@ -147,13 +147,13 @@ int drmRandomDestroy(void *state) unsigned long drmRandom(void *state) { RandomState *s = (RandomState *)state; - long hi; - long lo; + unsigned long hi; + unsigned long lo; hi = s->seed / s->q; lo = s->seed % s->q; s->seed = s->a * lo - s->r * hi; - if (s->seed <= 0) s->seed += s->m; + if ((s->a * lo) <= (s->r * hi)) s->seed += s->m; return s->seed; } @@ -166,7 +166,7 @@ double drmRandomDouble(void *state) } #if RANDOM_MAIN -static void check_period(long seed) +static void check_period(unsigned long seed) { unsigned long count = 0; unsigned long initial; @@ -178,7 +178,7 @@ static void check_period(long seed) while (initial != drmRandom(state)) { if (!++count) break; } - printf("With seed of %10ld, period = %10lu (0x%08lx)\n", + printf("With seed of %10lu, period = %10lu (0x%08lx)\n", seed, count, count); drmRandomDestroy(state); } @@ -195,7 +195,7 @@ int main(void) } printf("After 10000 iterations: %lu (%lu expected): %s\n", rand, state->check, - rand - state->check ? "*INCORRECT*" : "CORRECT"); + rand != state->check ? "*INCORRECT*" : "CORRECT"); drmRandomDestroy(state); printf("Checking periods...\n"); -- 2.1.0 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [libdrm][PATCH 2/2] Fix gcc -Wextra warnings 2015-02-09 21:39 [libdrm][PATCH 1/2] random: Use unsigned long for seed Jan Vesely @ 2015-02-09 21:39 ` Jan Vesely 2015-02-09 23:12 ` Ian Romanick 2015-02-09 23:32 ` Emil Velikov 2015-02-09 23:11 ` [libdrm][PATCH 1/2] random: Use unsigned long for seed Ian Romanick 2015-02-10 0:10 ` [libdrm][PATCH 3/2] Fix always true comparison Jan Vesely 2 siblings, 2 replies; 16+ messages in thread From: Jan Vesely @ 2015-02-09 21:39 UTC (permalink / raw) To: dri-devel Signed-off-by: Jan Vesely <jan.vesely@rutgers.edu> --- tests/vbltest/vbltest.c | 3 ++- xf86drm.c | 4 ++-- xf86drmMode.c | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/tests/vbltest/vbltest.c b/tests/vbltest/vbltest.c index cdc1ef6..6e13c57 100644 --- a/tests/vbltest/vbltest.c +++ b/tests/vbltest/vbltest.c @@ -104,7 +104,8 @@ static void usage(char *name) int main(int argc, char **argv) { - int i, c, fd, ret; + unsigned i; + int c, fd, ret; char *modules[] = { "i915", "radeon", "nouveau", "vmwgfx", "exynos", "omapdrm", "tilcdc", "msm", "tegra" }; drmVBlank vbl; drmEventContext evctx; diff --git a/xf86drm.c b/xf86drm.c index 345325a..fb673b5 100644 --- a/xf86drm.c +++ b/xf86drm.c @@ -303,7 +303,7 @@ static int chown_check_return(const char *path, uid_t owner, gid_t group) * special file node with the major and minor numbers specified by \p dev and * parent directory if necessary and was called by root. */ -static int drmOpenDevice(long dev, int minor, int type) +static int drmOpenDevice(dev_t dev, int minor, int type) { stat_t st; const char *dev_name; @@ -2213,7 +2213,7 @@ int drmGetClient(int fd, int idx, int *auth, int *pid, int *uid, int drmGetStats(int fd, drmStatsT *stats) { drm_stats_t s; - int i; + unsigned i; if (drmIoctl(fd, DRM_IOCTL_GET_STATS, &s)) return -errno; diff --git a/xf86drmMode.c b/xf86drmMode.c index 60ce369..e3e599b 100644 --- a/xf86drmMode.c +++ b/xf86drmMode.c @@ -854,7 +854,7 @@ int drmHandleEvent(int fd, drmEventContextPtr evctx) len = read(fd, buffer, sizeof buffer); if (len == 0) return 0; - if (len < sizeof *e) + if (len < (int)sizeof *e) return -1; i = 0; -- 2.1.0 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [libdrm][PATCH 2/2] Fix gcc -Wextra warnings 2015-02-09 21:39 ` [libdrm][PATCH 2/2] Fix gcc -Wextra warnings Jan Vesely @ 2015-02-09 23:12 ` Ian Romanick 2015-02-09 23:32 ` Emil Velikov 1 sibling, 0 replies; 16+ messages in thread From: Ian Romanick @ 2015-02-09 23:12 UTC (permalink / raw) To: Jan Vesely, dri-devel This patch is Reviewed-by: Ian Romanick <ian.d.romanick@intel.com> On 02/09/2015 01:39 PM, Jan Vesely wrote: > Signed-off-by: Jan Vesely <jan.vesely@rutgers.edu> > --- > tests/vbltest/vbltest.c | 3 ++- > xf86drm.c | 4 ++-- > xf86drmMode.c | 2 +- > 3 files changed, 5 insertions(+), 4 deletions(-) > > diff --git a/tests/vbltest/vbltest.c b/tests/vbltest/vbltest.c > index cdc1ef6..6e13c57 100644 > --- a/tests/vbltest/vbltest.c > +++ b/tests/vbltest/vbltest.c > @@ -104,7 +104,8 @@ static void usage(char *name) > > int main(int argc, char **argv) > { > - int i, c, fd, ret; > + unsigned i; > + int c, fd, ret; > char *modules[] = { "i915", "radeon", "nouveau", "vmwgfx", "exynos", "omapdrm", "tilcdc", "msm", "tegra" }; > drmVBlank vbl; > drmEventContext evctx; > diff --git a/xf86drm.c b/xf86drm.c > index 345325a..fb673b5 100644 > --- a/xf86drm.c > +++ b/xf86drm.c > @@ -303,7 +303,7 @@ static int chown_check_return(const char *path, uid_t owner, gid_t group) > * special file node with the major and minor numbers specified by \p dev and > * parent directory if necessary and was called by root. > */ > -static int drmOpenDevice(long dev, int minor, int type) > +static int drmOpenDevice(dev_t dev, int minor, int type) > { > stat_t st; > const char *dev_name; > @@ -2213,7 +2213,7 @@ int drmGetClient(int fd, int idx, int *auth, int *pid, int *uid, > int drmGetStats(int fd, drmStatsT *stats) > { > drm_stats_t s; > - int i; > + unsigned i; > > if (drmIoctl(fd, DRM_IOCTL_GET_STATS, &s)) > return -errno; > diff --git a/xf86drmMode.c b/xf86drmMode.c > index 60ce369..e3e599b 100644 > --- a/xf86drmMode.c > +++ b/xf86drmMode.c > @@ -854,7 +854,7 @@ int drmHandleEvent(int fd, drmEventContextPtr evctx) > len = read(fd, buffer, sizeof buffer); > if (len == 0) > return 0; > - if (len < sizeof *e) > + if (len < (int)sizeof *e) > return -1; > > i = 0; > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [libdrm][PATCH 2/2] Fix gcc -Wextra warnings 2015-02-09 21:39 ` [libdrm][PATCH 2/2] Fix gcc -Wextra warnings Jan Vesely 2015-02-09 23:12 ` Ian Romanick @ 2015-02-09 23:32 ` Emil Velikov 2015-02-10 0:02 ` Jan Vesely 1 sibling, 1 reply; 16+ messages in thread From: Emil Velikov @ 2015-02-09 23:32 UTC (permalink / raw) To: Jan Vesely; +Cc: ML dri-devel On 9 February 2015 at 21:39, Jan Vesely <jan.vesely@rutgers.edu> wrote: > Signed-off-by: Jan Vesely <jan.vesely@rutgers.edu> Nice one Jan. I've sent similar fixes for drmOpenDevice and drmGetStats a few days ago. Considering you drop the last hunk that Ian spotted both patches are Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> The strange part is that the normal Linux build does not show even a single warning, despite the -Wextra and -Wsign-compare flags from configure.ac. Perhaps my gcc does not like libdrm for some reason :P -Emil _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [libdrm][PATCH 2/2] Fix gcc -Wextra warnings 2015-02-09 23:32 ` Emil Velikov @ 2015-02-10 0:02 ` Jan Vesely 2015-02-10 0:27 ` Emil Velikov 0 siblings, 1 reply; 16+ messages in thread From: Jan Vesely @ 2015-02-10 0:02 UTC (permalink / raw) To: Emil Velikov; +Cc: ML dri-devel [-- Attachment #1.1: Type: text/plain, Size: 911 bytes --] On Mon, 2015-02-09 at 23:32 +0000, Emil Velikov wrote: > On 9 February 2015 at 21:39, Jan Vesely <jan.vesely@rutgers.edu> wrote: > > Signed-off-by: Jan Vesely <jan.vesely@rutgers.edu> > Nice one Jan. I've sent similar fixes for drmOpenDevice and > drmGetStats a few days ago. > > Considering you drop the last hunk that Ian spotted both patches are > Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> Thanks, I sent v2 of that patch few minutes ago. I think your 4/6 and 5/6 overlap with this one. Should I go ahead or do you plan to push yours? > > The strange part is that the normal Linux build does not show even a > single warning, despite the -Wextra and -Wsign-compare flags from > configure.ac. Perhaps my gcc does not like libdrm for some reason :P I think I just used CFLAGS= during configure, and it worked jan > > -Emil -- Jan Vesely <jan.vesely@rutgers.edu> [-- Attachment #1.2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 819 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [libdrm][PATCH 2/2] Fix gcc -Wextra warnings 2015-02-10 0:02 ` Jan Vesely @ 2015-02-10 0:27 ` Emil Velikov 2015-02-10 21:37 ` Jan Vesely 0 siblings, 1 reply; 16+ messages in thread From: Emil Velikov @ 2015-02-10 0:27 UTC (permalink / raw) To: Jan Vesely; +Cc: ML dri-devel On 10 February 2015 at 00:02, Jan Vesely <jan.vesely@rutgers.edu> wrote: > On Mon, 2015-02-09 at 23:32 +0000, Emil Velikov wrote: >> On 9 February 2015 at 21:39, Jan Vesely <jan.vesely@rutgers.edu> wrote: >> > Signed-off-by: Jan Vesely <jan.vesely@rutgers.edu> >> Nice one Jan. I've sent similar fixes for drmOpenDevice and >> drmGetStats a few days ago. >> >> Considering you drop the last hunk that Ian spotted both patches are >> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> > > Thanks, I sent v2 of that patch few minutes ago. > > I think your 4/6 and 5/6 overlap with this one. Should I go ahead or do > you plan to push yours? > I would go with your series - it handles more cases, plus already has move reviews :-P If you feel like looking through some of my series that would be appreciated. >> >> The strange part is that the normal Linux build does not show even a >> single warning, despite the -Wextra and -Wsign-compare flags from >> configure.ac. Perhaps my gcc does not like libdrm for some reason :P > > I think I just used CFLAGS= during configure, and it worked > jan > Yes the issue was that we're not using WARN_CFLAGS in every Makefile in libdrm. Namely the top one and most of the tests. -Emil _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [libdrm][PATCH 2/2] Fix gcc -Wextra warnings 2015-02-10 0:27 ` Emil Velikov @ 2015-02-10 21:37 ` Jan Vesely 2015-02-10 22:55 ` Emil Velikov 0 siblings, 1 reply; 16+ messages in thread From: Jan Vesely @ 2015-02-10 21:37 UTC (permalink / raw) To: Emil Velikov; +Cc: ML dri-devel [-- Attachment #1.1: Type: text/plain, Size: 2481 bytes --] On Tue, 2015-02-10 at 00:27 +0000, Emil Velikov wrote: > On 10 February 2015 at 00:02, Jan Vesely <jan.vesely@rutgers.edu> wrote: > > On Mon, 2015-02-09 at 23:32 +0000, Emil Velikov wrote: > >> On 9 February 2015 at 21:39, Jan Vesely <jan.vesely@rutgers.edu> wrote: > >> > Signed-off-by: Jan Vesely <jan.vesely@rutgers.edu> > >> Nice one Jan. I've sent similar fixes for drmOpenDevice and > >> drmGetStats a few days ago. > >> > >> Considering you drop the last hunk that Ian spotted both patches are > >> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> > > > > Thanks, I sent v2 of that patch few minutes ago. > > > > I think your 4/6 and 5/6 overlap with this one. Should I go ahead or do > > you plan to push yours? > > > I would go with your series - it handles more cases, plus already has > move reviews :-P > If you feel like looking through some of my series that would be appreciated. I wasn't subscribed to the list so I can't reply to those emails (don't know the message-ids). I looked at the series from Jan 29th [0]. 1/6[1], there is no tests/util directory, I guess it depends on Thierry's series? since it hasn't landed yet does it make sense to squash it there (like your 04.1/11 SQUASH: tests: misc cleanups) ? 2/6[2], also does not apply cleanly (needs Thierry's 5/11), if you want to push a version rebased on master you can add Reviewed-by: Jan Vesely <jan.vesely@rutgers.edu> to that one 4/6 and 5/6 were superseded, and I don't know enough about android to look at the other two, but 6/6 looks trivial enough Acked-by: Jan Vesely <jan.vesely@rutgers.edu> with a small nit: Why keep two assignments to LOCAL_SHARED_LIBRARIES in intel/Android.mk ? regards, jan [0]http://lists.freedesktop.org/archives/dri-devel/2015-January/076456.html [1]http://lists.freedesktop.org/archives/dri-devel/2015-January/076457.html [2]http://lists.freedesktop.org/archives/dri-devel/2015-January/076458.html > > >> > >> The strange part is that the normal Linux build does not show even a > >> single warning, despite the -Wextra and -Wsign-compare flags from > >> configure.ac. Perhaps my gcc does not like libdrm for some reason :P > > > > I think I just used CFLAGS= during configure, and it worked > > jan > > > Yes the issue was that we're not using WARN_CFLAGS in every Makefile in libdrm. > Namely the top one and most of the tests. > > -Emil -- Jan Vesely <jan.vesely@rutgers.edu> [-- Attachment #1.2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 819 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [libdrm][PATCH 2/2] Fix gcc -Wextra warnings 2015-02-10 21:37 ` Jan Vesely @ 2015-02-10 22:55 ` Emil Velikov 0 siblings, 0 replies; 16+ messages in thread From: Emil Velikov @ 2015-02-10 22:55 UTC (permalink / raw) To: Jan Vesely; +Cc: emil.l.velikov, ML dri-devel On 10/02/15 21:37, Jan Vesely wrote: > On Tue, 2015-02-10 at 00:27 +0000, Emil Velikov wrote: >> On 10 February 2015 at 00:02, Jan Vesely <jan.vesely@rutgers.edu> wrote: >>> On Mon, 2015-02-09 at 23:32 +0000, Emil Velikov wrote: >>>> On 9 February 2015 at 21:39, Jan Vesely <jan.vesely@rutgers.edu> wrote: >>>>> Signed-off-by: Jan Vesely <jan.vesely@rutgers.edu> >>>> Nice one Jan. I've sent similar fixes for drmOpenDevice and >>>> drmGetStats a few days ago. >>>> >>>> Considering you drop the last hunk that Ian spotted both patches are >>>> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> >>> >>> Thanks, I sent v2 of that patch few minutes ago. >>> >>> I think your 4/6 and 5/6 overlap with this one. Should I go ahead or do >>> you plan to push yours? >>> >> I would go with your series - it handles more cases, plus already has >> move reviews :-P >> If you feel like looking through some of my series that would be appreciated. > > I wasn't subscribed to the list so I can't reply to those emails (don't > know the message-ids). > I looked at the series from Jan 29th [0]. > > 1/6[1], there is no tests/util directory, I guess it depends on > Thierry's series? since it hasn't landed yet does it make sense to > squash it there (like your 04.1/11 SQUASH: tests: misc cleanups) ? > > 2/6[2], also does not apply cleanly (needs Thierry's 5/11), if you want > to push a version rebased on master you can add > > Reviewed-by: Jan Vesely <jan.vesely@rutgers.edu> > to that one > I'll these on hold and revive as Theirry's series lands. > 4/6 and 5/6 were superseded, and I don't know enough about android to > look at the other two, but > > 6/6 looks trivial enough > Acked-by: Jan Vesely <jan.vesely@rutgers.edu> > > with a small nit: > Why keep two assignments to LOCAL_SHARED_LIBRARIES in intel/Android.mk ? > Good catch. I'm assuming that (a) either I messed up at cherry-picking the patch or (b) git got confused as their three does not have libpciaccess in the list. Thanks Emil _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [libdrm][PATCH 1/2] random: Use unsigned long for seed 2015-02-09 21:39 [libdrm][PATCH 1/2] random: Use unsigned long for seed Jan Vesely 2015-02-09 21:39 ` [libdrm][PATCH 2/2] Fix gcc -Wextra warnings Jan Vesely @ 2015-02-09 23:11 ` Ian Romanick 2015-02-09 23:26 ` Jan Vesely 2015-02-09 23:28 ` [libdrm][PATCH v2 " Jan Vesely 2015-02-10 0:10 ` [libdrm][PATCH 3/2] Fix always true comparison Jan Vesely 2 siblings, 2 replies; 16+ messages in thread From: Ian Romanick @ 2015-02-09 23:11 UTC (permalink / raw) To: Jan Vesely, dri-devel On 02/09/2015 01:39 PM, Jan Vesely wrote: > This is more consistent with the rest, and avoids potential undefined > behavior (signed overflow) in drmRandom() > > Signed-off-by: Jan Vesely <jan.vesely@rutgers.edu> > --- > xf86drmRandom.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/xf86drmRandom.c b/xf86drmRandom.c > index ecab9e2..a084b86 100644 > --- a/xf86drmRandom.c > +++ b/xf86drmRandom.c > @@ -98,7 +98,7 @@ typedef struct RandomState { > unsigned long q; /* m div a */ > unsigned long r; /* m mod a */ > unsigned long check; > - long seed; > + unsigned long seed; > } RandomState; > > #if RANDOM_MAIN > @@ -147,13 +147,13 @@ int drmRandomDestroy(void *state) > unsigned long drmRandom(void *state) > { > RandomState *s = (RandomState *)state; > - long hi; > - long lo; > + unsigned long hi; > + unsigned long lo; > > hi = s->seed / s->q; > lo = s->seed % s->q; > s->seed = s->a * lo - s->r * hi; > - if (s->seed <= 0) s->seed += s->m; > + if ((s->a * lo) <= (s->r * hi)) s->seed += s->m; This seems like an unrelated change. > > return s->seed; > } > @@ -166,7 +166,7 @@ double drmRandomDouble(void *state) > } > > #if RANDOM_MAIN > -static void check_period(long seed) > +static void check_period(unsigned long seed) > { > unsigned long count = 0; > unsigned long initial; > @@ -178,7 +178,7 @@ static void check_period(long seed) > while (initial != drmRandom(state)) { > if (!++count) break; > } > - printf("With seed of %10ld, period = %10lu (0x%08lx)\n", > + printf("With seed of %10lu, period = %10lu (0x%08lx)\n", > seed, count, count); > drmRandomDestroy(state); > } > @@ -195,7 +195,7 @@ int main(void) > } > printf("After 10000 iterations: %lu (%lu expected): %s\n", > rand, state->check, > - rand - state->check ? "*INCORRECT*" : "CORRECT"); > + rand != state->check ? "*INCORRECT*" : "CORRECT"); So does this. > drmRandomDestroy(state); > > printf("Checking periods...\n"); > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [libdrm][PATCH 1/2] random: Use unsigned long for seed 2015-02-09 23:11 ` [libdrm][PATCH 1/2] random: Use unsigned long for seed Ian Romanick @ 2015-02-09 23:26 ` Jan Vesely 2015-02-09 23:28 ` [libdrm][PATCH v2 " Jan Vesely 1 sibling, 0 replies; 16+ messages in thread From: Jan Vesely @ 2015-02-09 23:26 UTC (permalink / raw) To: Ian Romanick; +Cc: dri-devel [-- Attachment #1.1: Type: text/plain, Size: 2655 bytes --] On Mon, 2015-02-09 at 15:11 -0800, Ian Romanick wrote: > On 02/09/2015 01:39 PM, Jan Vesely wrote: > > This is more consistent with the rest, and avoids potential undefined > > behavior (signed overflow) in drmRandom() > > > > Signed-off-by: Jan Vesely <jan.vesely@rutgers.edu> > > --- > > xf86drmRandom.c | 14 +++++++------- > > 1 file changed, 7 insertions(+), 7 deletions(-) > > > > diff --git a/xf86drmRandom.c b/xf86drmRandom.c > > index ecab9e2..a084b86 100644 > > --- a/xf86drmRandom.c > > +++ b/xf86drmRandom.c > > @@ -98,7 +98,7 @@ typedef struct RandomState { > > unsigned long q; /* m div a */ > > unsigned long r; /* m mod a */ > > unsigned long check; > > - long seed; > > + unsigned long seed; > > } RandomState; > > > > #if RANDOM_MAIN > > @@ -147,13 +147,13 @@ int drmRandomDestroy(void *state) > > unsigned long drmRandom(void *state) > > { > > RandomState *s = (RandomState *)state; > > - long hi; > > - long lo; > > + unsigned long hi; > > + unsigned long lo; > > > > hi = s->seed / s->q; > > lo = s->seed % s->q; > > s->seed = s->a * lo - s->r * hi; > > - if (s->seed <= 0) s->seed += s->m; > > + if ((s->a * lo) <= (s->r * hi)) s->seed += s->m; > > This seems like an unrelated change. This change is necessary. since s->seed is unsigned now it cannot be < 0. also the result of s->seed op s->q is unsigned. > > > > > return s->seed; > > } > > @@ -166,7 +166,7 @@ double drmRandomDouble(void *state) > > } > > > > #if RANDOM_MAIN > > -static void check_period(long seed) > > +static void check_period(unsigned long seed) > > { > > unsigned long count = 0; > > unsigned long initial; > > @@ -178,7 +178,7 @@ static void check_period(long seed) > > while (initial != drmRandom(state)) { > > if (!++count) break; > > } > > - printf("With seed of %10ld, period = %10lu (0x%08lx)\n", > > + printf("With seed of %10lu, period = %10lu (0x%08lx)\n", > > seed, count, count); > > drmRandomDestroy(state); > > } > > @@ -195,7 +195,7 @@ int main(void) > > } > > printf("After 10000 iterations: %lu (%lu expected): %s\n", > > rand, state->check, > > - rand - state->check ? "*INCORRECT*" : "CORRECT"); > > + rand != state->check ? "*INCORRECT*" : "CORRECT"); > > So does this. I'll drop this one in v2, it shouldn't have been included > > > drmRandomDestroy(state); > > > > printf("Checking periods...\n"); > > > -- Jan Vesely <jan.vesely@rutgers.edu> [-- Attachment #1.2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 819 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* [libdrm][PATCH v2 1/2] random: Use unsigned long for seed 2015-02-09 23:11 ` [libdrm][PATCH 1/2] random: Use unsigned long for seed Ian Romanick 2015-02-09 23:26 ` Jan Vesely @ 2015-02-09 23:28 ` Jan Vesely 2015-02-10 18:21 ` Ian Romanick 1 sibling, 1 reply; 16+ messages in thread From: Jan Vesely @ 2015-02-09 23:28 UTC (permalink / raw) To: dri-devel, Ian Romanick v2: Remove unrelated change in main() This is more consistent with the rest, and avoids potential undefined behavior (signed overflow) ind drmRandom() Signed-off-by: Jan Vesely <jan.vesely@rutgers.edu> --- xf86drmRandom.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/xf86drmRandom.c b/xf86drmRandom.c index ecab9e2..94922ad 100644 --- a/xf86drmRandom.c +++ b/xf86drmRandom.c @@ -98,7 +98,7 @@ typedef struct RandomState { unsigned long q; /* m div a */ unsigned long r; /* m mod a */ unsigned long check; - long seed; + unsigned long seed; } RandomState; #if RANDOM_MAIN @@ -147,13 +147,13 @@ int drmRandomDestroy(void *state) unsigned long drmRandom(void *state) { RandomState *s = (RandomState *)state; - long hi; - long lo; + unsigned long hi; + unsigned long lo; hi = s->seed / s->q; lo = s->seed % s->q; s->seed = s->a * lo - s->r * hi; - if (s->seed <= 0) s->seed += s->m; + if ((s->a * lo) <= (s->r * hi)) s->seed += s->m; return s->seed; } @@ -166,7 +166,7 @@ double drmRandomDouble(void *state) } #if RANDOM_MAIN -static void check_period(long seed) +static void check_period(unsigned long seed) { unsigned long count = 0; unsigned long initial; @@ -178,7 +178,7 @@ static void check_period(long seed) while (initial != drmRandom(state)) { if (!++count) break; } - printf("With seed of %10ld, period = %10lu (0x%08lx)\n", + printf("With seed of %10lu, period = %10lu (0x%08lx)\n", seed, count, count); drmRandomDestroy(state); } -- 2.1.0 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [libdrm][PATCH v2 1/2] random: Use unsigned long for seed 2015-02-09 23:28 ` [libdrm][PATCH v2 " Jan Vesely @ 2015-02-10 18:21 ` Ian Romanick 0 siblings, 0 replies; 16+ messages in thread From: Ian Romanick @ 2015-02-10 18:21 UTC (permalink / raw) To: Jan Vesely, dri-devel This patch is Reviewed-by: Ian Romanick <ian.d.romanick@intel.com> On 02/09/2015 03:28 PM, Jan Vesely wrote: > v2: Remove unrelated change in main() > > This is more consistent with the rest, and avoids potential undefined > behavior (signed overflow) ind drmRandom() > > Signed-off-by: Jan Vesely <jan.vesely@rutgers.edu> > --- > xf86drmRandom.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/xf86drmRandom.c b/xf86drmRandom.c > index ecab9e2..94922ad 100644 > --- a/xf86drmRandom.c > +++ b/xf86drmRandom.c > @@ -98,7 +98,7 @@ typedef struct RandomState { > unsigned long q; /* m div a */ > unsigned long r; /* m mod a */ > unsigned long check; > - long seed; > + unsigned long seed; > } RandomState; > > #if RANDOM_MAIN > @@ -147,13 +147,13 @@ int drmRandomDestroy(void *state) > unsigned long drmRandom(void *state) > { > RandomState *s = (RandomState *)state; > - long hi; > - long lo; > + unsigned long hi; > + unsigned long lo; > > hi = s->seed / s->q; > lo = s->seed % s->q; > s->seed = s->a * lo - s->r * hi; > - if (s->seed <= 0) s->seed += s->m; > + if ((s->a * lo) <= (s->r * hi)) s->seed += s->m; > > return s->seed; > } > @@ -166,7 +166,7 @@ double drmRandomDouble(void *state) > } > > #if RANDOM_MAIN > -static void check_period(long seed) > +static void check_period(unsigned long seed) > { > unsigned long count = 0; > unsigned long initial; > @@ -178,7 +178,7 @@ static void check_period(long seed) > while (initial != drmRandom(state)) { > if (!++count) break; > } > - printf("With seed of %10ld, period = %10lu (0x%08lx)\n", > + printf("With seed of %10lu, period = %10lu (0x%08lx)\n", > seed, count, count); > drmRandomDestroy(state); > } > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* [libdrm][PATCH 3/2] Fix always true comparison. 2015-02-09 21:39 [libdrm][PATCH 1/2] random: Use unsigned long for seed Jan Vesely 2015-02-09 21:39 ` [libdrm][PATCH 2/2] Fix gcc -Wextra warnings Jan Vesely 2015-02-09 23:11 ` [libdrm][PATCH 1/2] random: Use unsigned long for seed Ian Romanick @ 2015-02-10 0:10 ` Jan Vesely 2015-02-25 17:11 ` Jan Vesely 2 siblings, 1 reply; 16+ messages in thread From: Jan Vesely @ 2015-02-10 0:10 UTC (permalink / raw) To: dri-devel, Ian Romanick, Emil Velikov The only user I found is xserver, it can return -1 under certain conditions. So check for -1 explicitly. Signed-off-by: Jan Vesely <jan.vesely@rutgers.edu> --- I could not find whether it's actually legal to return encoded negative values in get_perm. This is a quick fix to detect the one case that I found. xf86drm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xf86drm.c b/xf86drm.c index fb673b5..8e54ac9 100644 --- a/xf86drm.c +++ b/xf86drm.c @@ -335,7 +335,7 @@ static int drmOpenDevice(dev_t dev, int minor, int type) drm_server_info->get_perms(&serv_group, &serv_mode); devmode = serv_mode ? serv_mode : DRM_DEV_MODE; devmode &= ~(S_IXUSR|S_IXGRP|S_IXOTH); - group = (serv_group >= 0) ? serv_group : DRM_DEV_GID; + group = (serv_group != ~0U) ? serv_group : DRM_DEV_GID; } #if !defined(UDEV) -- 2.1.0 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [libdrm][PATCH 3/2] Fix always true comparison. 2015-02-10 0:10 ` [libdrm][PATCH 3/2] Fix always true comparison Jan Vesely @ 2015-02-25 17:11 ` Jan Vesely 2015-02-25 18:41 ` Emil Velikov 0 siblings, 1 reply; 16+ messages in thread From: Jan Vesely @ 2015-02-25 17:11 UTC (permalink / raw) To: dri-devel; +Cc: Emil Velikov [-- Attachment #1.1: Type: text/plain, Size: 1032 bytes --] gentle ping On Mon, 2015-02-09 at 19:10 -0500, Jan Vesely wrote: > The only user I found is xserver, it can return -1 under certain conditions. > So check for -1 explicitly. > > Signed-off-by: Jan Vesely <jan.vesely@rutgers.edu> > --- > > I could not find whether it's actually legal to return encoded negative values > in get_perm. This is a quick fix to detect the one case that I found. > > xf86drm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/xf86drm.c b/xf86drm.c > index fb673b5..8e54ac9 100644 > --- a/xf86drm.c > +++ b/xf86drm.c > @@ -335,7 +335,7 @@ static int drmOpenDevice(dev_t dev, int minor, int type) > drm_server_info->get_perms(&serv_group, &serv_mode); > devmode = serv_mode ? serv_mode : DRM_DEV_MODE; > devmode &= ~(S_IXUSR|S_IXGRP|S_IXOTH); > - group = (serv_group >= 0) ? serv_group : DRM_DEV_GID; > + group = (serv_group != ~0U) ? serv_group : DRM_DEV_GID; > } > > #if !defined(UDEV) -- Jan Vesely <jan.vesely@rutgers.edu> [-- Attachment #1.2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 819 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [libdrm][PATCH 3/2] Fix always true comparison. 2015-02-25 17:11 ` Jan Vesely @ 2015-02-25 18:41 ` Emil Velikov 2015-03-02 20:01 ` Jan Vesely 0 siblings, 1 reply; 16+ messages in thread From: Emil Velikov @ 2015-02-25 18:41 UTC (permalink / raw) To: Jan Vesely, dri-devel; +Cc: emil.l.velikov On 25/02/15 17:11, Jan Vesely wrote: > gentle ping > Afaics it's very had to get in this code nowadays - drm_server_info is set only via the legacy (?) function drmSetServerInfo. With the latter only(?) used by the xserver when working with dri1 modules. So testing this is likely to be very painful :-( This code hasn't changed since before 2007, so I doubt there are many people that know the details about it, so we might as well leave it for now ? -Emil > > On Mon, 2015-02-09 at 19:10 -0500, Jan Vesely wrote: >> The only user I found is xserver, it can return -1 under certain conditions. >> So check for -1 explicitly. >> >> Signed-off-by: Jan Vesely <jan.vesely@rutgers.edu> >> --- >> >> I could not find whether it's actually legal to return encoded negative values >> in get_perm. This is a quick fix to detect the one case that I found. >> >> xf86drm.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/xf86drm.c b/xf86drm.c >> index fb673b5..8e54ac9 100644 >> --- a/xf86drm.c >> +++ b/xf86drm.c >> @@ -335,7 +335,7 @@ static int drmOpenDevice(dev_t dev, int minor, int type) >> drm_server_info->get_perms(&serv_group, &serv_mode); >> devmode = serv_mode ? serv_mode : DRM_DEV_MODE; >> devmode &= ~(S_IXUSR|S_IXGRP|S_IXOTH); >> - group = (serv_group >= 0) ? serv_group : DRM_DEV_GID; >> + group = (serv_group != ~0U) ? serv_group : DRM_DEV_GID; >> } >> >> #if !defined(UDEV) > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [libdrm][PATCH 3/2] Fix always true comparison. 2015-02-25 18:41 ` Emil Velikov @ 2015-03-02 20:01 ` Jan Vesely 0 siblings, 0 replies; 16+ messages in thread From: Jan Vesely @ 2015-03-02 20:01 UTC (permalink / raw) To: Emil Velikov; +Cc: dri-devel [-- Attachment #1.1: Type: text/plain, Size: 1783 bytes --] On Wed, 2015-02-25 at 18:41 +0000, Emil Velikov wrote: > On 25/02/15 17:11, Jan Vesely wrote: > > gentle ping > > > Afaics it's very had to get in this code nowadays - drm_server_info is > set only via the legacy (?) function drmSetServerInfo. With the latter > only(?) used by the xserver when working with dri1 modules. So testing > this is likely to be very painful :-( > > This code hasn't changed since before 2007, so I doubt there are many > people that know the details about it, so we might as well leave it for > now ? fair enough. the warning fixes push it to non-libudev side, so it's not going to bug me on every build. jan > > -Emil > > > > > On Mon, 2015-02-09 at 19:10 -0500, Jan Vesely wrote: > >> The only user I found is xserver, it can return -1 under certain conditions. > >> So check for -1 explicitly. > >> > >> Signed-off-by: Jan Vesely <jan.vesely@rutgers.edu> > >> --- > >> > >> I could not find whether it's actually legal to return encoded negative values > >> in get_perm. This is a quick fix to detect the one case that I found. > >> > >> xf86drm.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/xf86drm.c b/xf86drm.c > >> index fb673b5..8e54ac9 100644 > >> --- a/xf86drm.c > >> +++ b/xf86drm.c > >> @@ -335,7 +335,7 @@ static int drmOpenDevice(dev_t dev, int minor, int type) > >> drm_server_info->get_perms(&serv_group, &serv_mode); > >> devmode = serv_mode ? serv_mode : DRM_DEV_MODE; > >> devmode &= ~(S_IXUSR|S_IXGRP|S_IXOTH); > >> - group = (serv_group >= 0) ? serv_group : DRM_DEV_GID; > >> + group = (serv_group != ~0U) ? serv_group : DRM_DEV_GID; > >> } > >> > >> #if !defined(UDEV) > > > -- Jan Vesely <jan.vesely@rutgers.edu> [-- Attachment #1.2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 819 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2015-03-02 20:01 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-02-09 21:39 [libdrm][PATCH 1/2] random: Use unsigned long for seed Jan Vesely 2015-02-09 21:39 ` [libdrm][PATCH 2/2] Fix gcc -Wextra warnings Jan Vesely 2015-02-09 23:12 ` Ian Romanick 2015-02-09 23:32 ` Emil Velikov 2015-02-10 0:02 ` Jan Vesely 2015-02-10 0:27 ` Emil Velikov 2015-02-10 21:37 ` Jan Vesely 2015-02-10 22:55 ` Emil Velikov 2015-02-09 23:11 ` [libdrm][PATCH 1/2] random: Use unsigned long for seed Ian Romanick 2015-02-09 23:26 ` Jan Vesely 2015-02-09 23:28 ` [libdrm][PATCH v2 " Jan Vesely 2015-02-10 18:21 ` Ian Romanick 2015-02-10 0:10 ` [libdrm][PATCH 3/2] Fix always true comparison Jan Vesely 2015-02-25 17:11 ` Jan Vesely 2015-02-25 18:41 ` Emil Velikov 2015-03-02 20:01 ` Jan Vesely
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.