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
