Bug 110678

Summary: Split "assert (a && b)" statements into "assert(a); assert(b)", for more precise diagnostics
Product: xorg Reporter: Adam J. Richter <adam_richter2004>
Component: Driver/intelAssignee: Chris Wilson <chris>
Status: RESOLVED MOVED QA Contact: Intel GFX Bugs mailing list <intel-gfx-bugs>
Severity: enhancement    
Priority: low CC: adam_richter2004
Version: unspecifiedKeywords: patch
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments:
Description Flags
Patch to split assert(a && b) statements into assert(a) and assert(b) in git version of xf86-video-intel none

Description Adam J. Richter 2019-05-14 22:09:32 UTC
Created attachment 144271 [details] [review]
Patch to split assert(a && b) statements into assert(a) and assert(b) in git version of xf86-video-intel

Many thanks to Adam Jackson via Maarten Maathuis for redirecting me to submit this patch here.

This patch separates each statement of the form assert(a && b) into "assert(a)" and "assert(b)" statements.

If you will excuse the boilerplate language, here are some arguments in favor of this practice.  Thanks for considering this patch.


1. Assertion failures are often sporadic, and users who report them may
   not be in a position to efficiently narrow them down further, so it
   is important to get as much precision from each assertion failure as
   possible.

2. It is a more efficient use of developers time when a bug report
   arrives if the problem has been narrowed down that much more.  A
   new bug report may initially attract more interest, so, if the
   problem has been narrowed down that much more, it may increase the
   chance that developers may invest the time to try to resolve the
   problem, and also reduce unnecessary traffic on the developer mailing
   list about possible causes of the bug that separating the assertion
   was able to rule out.
   
3. When using a debugger to step over an assertion failure in the
   first part of the statement, the second part is still tested.

4. Providing separate likelihood hints to the compiler in the form
   of separate assert statements does not require the compiler to
   be quite as smart to recognize that it should optimize both branches,
   although I do not know if that makes a difference for any compiler
   commonly used to compile X (that is, I suspect that they are all
   smart enough to realize is that "a && b" is likely true, then "a"
   is likely true and "b" is likely true).

5. In my humble opinion, separated assertions are almost always more
   readable, sometimes eliminating parentheses, often making
   references to the same values line up on the same columns, which
   can make range checks more obvious, and sometimes changing
   multi-line conditions to separate single line conditions, which can
   be recognized separately.

   Consider, for example, how columnization in this makes this range
   check more recongizable and makes it easier to recognize in a glance
   that it is a bounds check and what the bounds are:

                assert(reg.vstride >= 0 && reg.vstride < ARRAY_SIZE(vstride_for_reg));
                        ... vs. ...

                assert(reg.vstride >= 0);
                assert(reg.vstride < ARRAY_SIZE(vstride_for_reg));

   A possible counter-argument to this might be that, in a sequence of
   assertions, can be informative to group related assertions together,
   which I think is true, but it is possible to get this other means, such
   as by using blank lines to separate express such grouping.
Comment 1 Chris Wilson 2019-05-15 07:04:12 UTC
Whereas I agree with the sentiment, my spot checking shows that logical tests have been split which do not provide any more information. They do not provide any of the information that is provided in the trace, and are basically a means to narrow down the trace.

These asserts should never be enabled by default, or at any time other than during debugging as they actively destabilize the system by breaking on the first sign of error that would otherwise be handed normally. They *cause* data loss.

Since you are debugging, going one step further to enable the trace or attach gdb and inspect the values is not onerous. So I have to say the patch is mere churn that doesn't improve debugging one iota.
Comment 2 Martin Peres 2019-11-27 13:50:22 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/xorg/driver/xf86-video-intel/issues/163.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.