On Mon, Oct 05, 2015 at 05:59:50PM +0100, Michel Thierry wrote:
> On 10/5/2015 5:36 PM, Dave Gordon wrote:
> >On 02/10/15 14:16, Michel Thierry wrote:
> >>We tried to fix this in commit fdc454c1484a ("drm/i915: Prevent out of
> >>range pt in gen6_for_each_pde").
> >>
> >>But the static analyzer still complains that, just before we break due
> >>to "iter < I915_PDES", we do "pt = (pd)->page_table[iter]" with an
> >>iter value that is bigger than I915_PDES. Of course, this isn't really
> >>a problem since no one uses pt outside the macro. Still, every single
> >>new usage of the macro will create a new issue for us to mark as a
> >>false positive.
> >>
> >>Also, Paulo re-started the discussion a while ago [1], but didn't end up
> >>implemented.
> >>
> >>In order to "solve" this "problem", this patch takes the ideas from
> >>Chris and Dave, but that check would change the desired behavior of the
> >>code, because the object (for example pdp->page_directory[iter]) can be
> >>null during init/alloc, and C would take this as false, breaking the for
> >>loop immediately.
> >>
> >>This has been already verified with "static analysis tools".
> >>
> >>[1]http://lists.freedesktop.org/archives/intel-gfx/2015-June/068548.html
> >>
> >>v2: Make it a single statement, while preventing the common subexpression
> >>elimination (Chris)
> >>
> >>Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> >>Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> >>Cc: Dave Gordon <david.s.gordon@xxxxxxxxx>
> >>Signed-off-by: Michel Thierry <michel.thierry@xxxxxxxxx>
> >>---
> >> drivers/gpu/drm/i915/i915_gem_gtt.h | 14 ++++++++------
> >> 1 file changed, 8 insertions(+), 6 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h
> >>b/drivers/gpu/drm/i915/i915_gem_gtt.h
> >>index 9fbb07d..a216397 100644
> >>--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> >>+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> >>@@ -394,7 +394,8 @@ struct i915_hw_ppgtt {
> >> */
> >> #define gen6_for_each_pde(pt, pd, start, length, temp, iter) \
> >> for (iter = gen6_pde_index(start); \
> >>- pt = (pd)->page_table[iter], length > 0 && iter < I915_PDES; \
> >>+ length > 0 && iter < I915_PDES ? \
> >>+ (pt = (pd)->page_table[iter]), 1 : 0; \
> >> iter++, \
> >> temp = ALIGN(start+1, 1 << GEN6_PDE_SHIFT) - start, \
> >> temp = min_t(unsigned, temp, length), \
> >>@@ -459,7 +460,8 @@ static inline uint32_t gen6_pde_index(uint32_t addr)
> >> */
> >> #define gen8_for_each_pde(pt, pd, start, length, temp, iter) \
> >> for (iter = gen8_pde_index(start); \
> >>- pt = (pd)->page_table[iter], length > 0 && iter <
> >>I915_PDES; \
> >>+ length > 0 && iter < I915_PDES ? \
> >>+ (pt = (pd)->page_table[iter]), 1 : 0; \
> >> iter++, \
> >> temp = ALIGN(start+1, 1 << GEN8_PDE_SHIFT) - start, \
> >> temp = min(temp, length), \
> >>@@ -467,8 +469,8 @@ static inline uint32_t gen6_pde_index(uint32_t addr)
> >>
> >> #define gen8_for_each_pdpe(pd, pdp, start, length, temp, iter) \
> >> for (iter = gen8_pdpe_index(start); \
> >>- pd = (pdp)->page_directory[iter], \
> >>- length > 0 && (iter < I915_PDPES_PER_PDP(dev)); \
> >>+ length > 0 && (iter < I915_PDPES_PER_PDP(dev)) ? \
> >>+ (pd = (pdp)->page_directory[iter]), 1 : 0; \
> >> iter++, \
> >> temp = ALIGN(start+1, 1 << GEN8_PDPE_SHIFT) - start, \
> >> temp = min(temp, length), \
> >>@@ -476,8 +478,8 @@ static inline uint32_t gen6_pde_index(uint32_t addr)
> >>
> >> #define gen8_for_each_pml4e(pdp, pml4, start, length, temp, iter) \
> >> for (iter = gen8_pml4e_index(start); \
> >>- pdp = (pml4)->pdps[iter], \
> >>- length > 0 && iter < GEN8_PML4ES_PER_PML4; \
> >>+ length > 0 && iter < GEN8_PML4ES_PER_PML4 ? \
> >>+ (pdp = (pml4)->pdps[iter]), 1 : 0; \
> >
> >this won't compile -- see below
>
> Hmm, it compiled (also got rid of of the "analysis tool error" and didn't
> see any behavior change).
>
> >
> >> iter++, \
> >> temp = ALIGN(start+1, 1ULL << GEN8_PML4E_SHIFT) - start, \
> >> temp = min(temp, length), \
> >
> >The man page for C operators tells us:
> >
> > Operator Associativity
> > () [] -> . left to right
> > ! ~ ++ -- + - (type) * & sizeof right to left
> > * / % left to right
> > + - left to right
> > << >> left to right
> > < <= > >= left to right
> > == != left to right
> > & left to right
> > ^ left to right
> > | left to right
> > && left to right
> > || left to right
> > ?: right to left
> > = += -= *= /= %= <<= >>= &= ^= |= right to left
> > , left to right
> >
> >So there's a problem with the above code, because the comma operator is
> >LOWER precedence than either assignment or ?: You'd need to put the
> >parentheses around the (pdp = ... , 1) section, not just the assignment.
> >
> >Or for yet another variation, how about:
> >
> >#define gen8_for_each_pdpe(pd, pdp, start, length, temp, iter) \
> > for (iter = gen8_pdpe_index(start); \
> > iter < I915_PDPES_PER_PDP(dev) && \
> > (pd = (pdp)->page_directory[iter], length > 0); \
> > iter++, \
> > temp = ALIGN(start+1, 1 << GEN8_PDPE_SHIFT) - start, \
> > temp = min(temp, length), \
> > start += temp, length -= temp)
> >
> >#define gen8_for_each_pml4e(pdp, pml4, start, length, temp, iter) \
> > for (iter = gen8_pml4e_index(start); \
> > iter < GEN8_PML4ES_PER_PML4 && \
> > (pdp = (pml4)->pdps[iter], length > 0); \
> > iter++, \
> > temp = ALIGN(start+1, 1ULL << GEN8_PML4E_SHIFT) - start, \
> > temp = min(temp, length), \
> > start += temp, length -= temp)
> >
> >with no ugly ?: at all :)
>
> This also works.
>
> Since it's a change for not a real issue, I don't have a preference.
> I can send either.
Yeah, since ?: is a ternary operator parsing implicitly adds the () in the
middle and always parses it as a ? (b) : c. If lower-level operators in
the middle could split the ternary operator then it would result in
parsing fail (sinc ? without the : is just useless). So lgtm. Someone
willing to smack an r-b onto the patch?
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
