Re: [PATCH] Update ENABLE_CHECKING to make it usable in "if" conditions

[Richard Biener <richard dot guenther at gmail dot com>]

On Wed, Sep 9, 2015 at 11:07 PM, Jeff Law <law@redhat.com> wrote:
> On 08/31/2015 05:30 AM, Richard Biener wrote:
>>
>> On Mon, Aug 31, 2015 at 7:49 AM, Mikhail Maltsev <maltsevm@gmail.com>
>> 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.
>>>
>>> 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).
>>
>>
>> I think we want to keep ENABLE_CHECKING for macro use and for some
>> selected
>> cases.
>
> Can you outline which cases you want to keep?  My general feeling is to
> avoid conditionally compiled code as much as we can.

I guess I was merely looking for the patch to be split up to see the motivation
of a always-defined CHECKING_P macro.  With the suggestion to have
a runtime flag_checking variable I wonder if there is any real code that
is guarded by ENABLE_CHECKING now that can't use flag_checking
(yes, tree checking macros and gcc_checking_assert, but those already
work with ENABLE_CHECKING just fine and are in macro context anyway).

Richard.

>>
>>> 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
>
> We'll eventually want to do the reindentations as well, but for an RFC,
> that's fine.
>
>
>>>
>>> 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.
>>
>>
>> You should open a bugreport.
>
> Agreed.  Looks like a simple bug.
>
>>
>>> In lra.c we have:
>>>
>>> #ifdef ENABLE_CHECKING
>>>
>>>   /* Function checks RTL for correctness. If FINAL_P is true, it is
>>>      done at the end of LRA and the check is more rigorous.  */
>>>   static void
>>>   check_rtl (bool final_p)
>>> ...
>>> #ifdef ENABLED_CHECKING
>>>              extract_constrain_insn (insn);
>>> #endif
>>> ...
>>> #endif /* #ifdef ENABLE_CHECKING */
>>>
>>> The comment for extract_constrain_insn says:
>>> /* Do uncached extract_insn, constrain_operands and complain about
>>> failures.
>>>     This should be used when extracting a pre-existing constrained
>>> instruction
>>>     if the caller wants to know which alternative was chosen.  */
>>>
>>> So, as I understand, this additional check can be useful. This patch
>>> removes
>>> "#ifdef ENABLED_CHECKING" (without regressions).
>
> No strong opinions here.  There's other things that would catch this later.
> The check merely catches it closer to the point where things go wrong.  So
> I'd tend to want it to be conditional.
>
>>>
>>> The third issue is that some gcc_checking_assert-s were guarded by #ifdef
>>> like:
>>> #ifdef ENABLE_CHECKING
>>>     gcc_checking_assert (!is_deleted (*slot));
>>> #endif
>>> (is_deleted is defined unconditionally).
>>>
>>> Probably that is because it is not obvious from the "release" definition
>>> #define gcc_checking_assert(EXPR) ((void)(0 && (EXPR)))
>>> that EXPR is not evaluated.
>>>
>>> At least, I first decided to change it to something like
>>> "do { if (0) (void)(EXPR); } while(0)"
>>> and only then realized that the effect of "0 &&" is exactly the same. I
>>> added a
>>> comment to make it more obvious.
>
> Sounds reasonable.
>
>>>
>>> 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.
>>
>>
>> Erm.  Ok so if you are here...  I've long wanted the heavier parts of
>> checking
>> (like IL checking) to be controlled by a command-line switch (-fchecking
>> or
>> even -fchecking[=...]) rather than being compile-time controlled.
>> Obviously
>> that's not sth we want (or easily can get) for things like tree-checking
>> or
>> asserts.
>>
>> Just a suggestion which might eventually remove a whole bunch of
>> ENABLE_CHECKING #ifs before tackling the rest.
>
> Seems like a reasonable thing to me.
>
> jeff
>