Update ENABLE_CHECKING to make it usable in "if" conditions

On 08/30/2015 11:49 PM, Mikhail Maltsev wrote:
> Hi, all!
>
> This patch removes some conditional compilation from GCC. In this patch I define
> a macro CHECKING_P, which is equal to 1 when ENABLE_CHECKING is defined and 0
> otherwise. The reason for using a new name is the following: currently in GCC
> there are many places where ENABLE_CHECKING is checked using #ifdef, and it is a
> common way to do such checks (though #if would also work and is used in several
> places). If we change it in such way that it is always defined, accidentally
> using "#ifdef" instead of "#if" will lead to subtle errors: some expensive
> checks intended only for development stage will be enabled in release build and
> cause performance degradation.
Understood.  I'd generally like to get away from #if conditionals for 
checking.


>
> This patch removes all uses of ENABLE_CHECKING (I also tried poisoning this
> identifier in system.h, and the build succeeded, but I don't know how will this
> affect, e.g. merging feature branches, so I think such decisions should be made
> by maintainers).
If you've totally eliminated ENABLE_CHECKING, then I think poisoning the 
identifier would be wise.  That would ensure that a merge from a branch 
to the trunk wouldn't ever be able to accidentally re-introduce 
ENABLE_CHECKING conditional code.


>
> As for conditional compilation, I tried to remove it and replace #ifdef-s with
> if-s where possible, but, of course, avoided changing data structures layout. I
> also avoided reindenting large blocks of code. I changed some functions (and a
> couple of global variables) that were only used in "checking" build so that now
> they are always defined/compiled and have a DEBUG_FUNCTION (i.e. "used")
> attribute. I'll try to handle them in a more clean way: move CHECKING_P check
> inside their definitions - that will slightly reduce "visual noise" from
> conditions like
Note that this can be a somewhat incremental change.  ie, once this 
patch goes in, we can iterate on the remaining conditionally compiled 
checking code.

FWIW, I think structures changing their size based on something like 
ENABLE_CHECKING is a mistake and we should correct those mistakes.  That 
can be iterated upon as well.

>
> While working on this patch I noticed some issues worth mentioning. In
> sese.h:bb_in_region we have a check (enabled only in "checking" build):
>
>        /* Check that there are no edges coming in the region: all the
> 	 predecessors of EXIT are dominated by ENTRY.  */
>        FOR_EACH_EDGE (e, ei, exit->preds)
> 	dominated_by_p (CDI_DOMINATORS, e->src, entry);
>
> IIUC, dominated_by_p has no side effects, so it's useless. Changing it to
> "gcc_assert (dominated_by_p (...));" causes regressions. I disabled it in the
> patch and added a comment.
As I mentioned in my reply to Richi, this looks like a bug.  I think it 
probably should have been

  gcc_assert (dominated_by_p (CDI_DOMINATORS, e->src, entry));

We should track down why this causes regressions :-)


   >
> I tested the patch on x86_64-pc-linux-gnu with --enable-checking=yes and
> "release". Likewise, built config-list.mk in both configurations. There we some
> minor changes since that check, but I'll need to rebase the patch anyway, so I
> will repeat the full check.
>
> Please review the patch.
For the future, it'd be helpful if patches like this could be split up a 
bit.  It just makes the whole review process easier.  We can (for 
example) approve the infrastructure bit as well as anything that is 
obviously correct and give feedback and iterate as needed on each 
logical grouping of code.

So for example, in this case, A patch which introduces CHECKING_P would 
be first.  Then a distinct patch for each front-end, then a patch for 
each target.  You could even split up the tree vs RTL stuff.  Last patch 
would poison ENABLE_CHECKING.   From what I've read so far (and I'm 
probably less than 10% through the patch), much of it could go in 
immediately.

In general, what's the rationale behind using gcc_checking_assert rather 
than gcc_assert?  When the expression doesn't have side effects it would 
seem like a gratutious change.

In general, watch formatting. Lines should not exceed 80 columns. 
There's exceptions, but they're rare.  Break lines before operators, not 
after them.  Sometimes it appears to be done correctly, other times it 
isn't.



   foo (some_really_long_thing
        == something_else_that_might_be_long_too)

In several places you've got

if (CHECKING_P)
   /* comment */
   line of code

That breaks the visual style of how GNU code is written.  Please use

/* comment */
if (CHECKING_P)
   line of code

or

if (CHECKING_P)
   {
     /* comment */
     line of code
   }

When you change from gcc_assert to gcc_checking_assert, you'll have to 
fix the formatting if there were line breaks in the arguments to line up 
at the new location (9 spaces further indented).

If you could go back over the things you've changed and look for those 
formatting issues, it'd be appreciated.

Overall I like the patch and would likely approve parts of it 
immediately if it were in pieces.   Parts with formatting nits I'd point 
out individually and pre-approve with formatting nits fixed.  That's 
hard to do with a very large patch like this.

Aside from the general formatting nits noted above, a couple additional 
nits: