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:

diff --git a/gcc/cfgcleanup.c b/gcc/cfgcleanup.c
index 7e576bc..8795eef 100644
--- a/gcc/cfgcleanup.c
+++ b/gcc/cfgcleanup.c
@@ -2875,9 +2875,8 @@ try_optimize_cfg (int mode)
                  which will be done in fixup_partitions.  */
               fixup_partitions ();

-#ifdef ENABLE_CHECKING
-              verify_flow_info ();
-#endif
+             if (CHECKING_P)
+               verify_flow_info ();
Looks like indentation is wrong here on the new code.

diff --git a/gcc/genconditions.c b/gcc/genconditions.c
index 001e58e..7481ab4 100644
--- a/gcc/genconditions.c
+++ b/gcc/genconditions.c
@@ -60,6 +60,8 @@ write_header (void)
 \n\
 /* Do not allow checking to confuse the issue.  */\n\
 #undef ENABLE_CHECKING\n\
Does this line need to be removed?

Jeff

Reply via email to