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
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 :)
.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
