Re: [PING][RFC] Assertions as optimization hints

2016-11-28 Thread Vincent Lefevre
On 2016-11-23 16:03:44 +, Yuri Gribov wrote:
> Definitely, aggressively abusing assertions like this should be a
> per-project decision. E.g. I've seen code which parallels assertions
> with error checking i.e. something like
>   FILE *p = fopen(...);
>   assert(p);  // Quickly abort in debug mode
>   if (!p)
> return ERROR;
> Enabling __ASSUME_ASSERTS__ for such code would effectively disable
> all safety checks.

Yes, the problem comes from the fact that ISO C provides only one
form of assertions. In GNU MPFR, we have two kinds of assertions
(different from the one used above), but they are not based on
assert():

   MPFR_ASSERTN(expr): assertions that should normally be checked,
 otherwise give a hint to the compiler.

   MPFR_ASSERTD(expr): assertions that should be checked when testing,
 otherwise give a hint to the compiler.

Basically, MPFR_ASSERTN() will be used to do some checks on data
provided by the caller (e.g. to detect some API misuse), and
MPFR_ASSERTD() is there to detect internal inconsistencies (i.e.
bugs in MPFR).

IMHO, this is the correct way to do, as one can then enables hints
without breaking code like the above one.

Concerning the "per-project decision", I'm not sure that's a good
idea: as soon as one includes a header file, __ASSUME_ASSERTS__
could potentially break code. Said otherwise, if the user wants
optimization hints, it should not be based on assert().

-- 
Vincent Lefèvre  - Web: 
100% accessible validated (X)HTML - Blog: 
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)


Re: [PING][RFC] Assertions as optimization hints

2016-11-28 Thread Yuri Gribov
On Mon, Nov 28, 2016 at 4:03 PM, Vincent Lefevre  wrote:
> On 2016-11-23 16:03:44 +, Yuri Gribov wrote:
>> Definitely, aggressively abusing assertions like this should be a
>> per-project decision. E.g. I've seen code which parallels assertions
>> with error checking i.e. something like
>>   FILE *p = fopen(...);
>>   assert(p);  // Quickly abort in debug mode
>>   if (!p)
>> return ERROR;
>> Enabling __ASSUME_ASSERTS__ for such code would effectively disable
>> all safety checks.
>
> Yes, the problem comes from the fact that ISO C provides only one
> form of assertions. In GNU MPFR, we have two kinds of assertions
> (different from the one used above), but they are not based on
> assert():
>
>MPFR_ASSERTN(expr): assertions that should normally be checked,
>  otherwise give a hint to the compiler.
>
>MPFR_ASSERTD(expr): assertions that should be checked when testing,
>  otherwise give a hint to the compiler.
>
> Basically, MPFR_ASSERTN() will be used to do some checks on data
> provided by the caller (e.g. to detect some API misuse), and
> MPFR_ASSERTD() is there to detect internal inconsistencies (i.e.
> bugs in MPFR).
>
> IMHO, this is the correct way to do, as one can then enables hints
> without breaking code like the above one.

Yes, many projects adopted similar approach. And it's indeed a pity
that standard does not distinguish between assume and assert so we
don't have universal mechanism to inform compiler about exploitable
code invariants.

> Concerning the "per-project decision", I'm not sure that's a good
> idea: as soon as one includes a header file, __ASSUME_ASSERTS__
> could potentially break code.

Sorry, not sure I understood. __ASSUME_ASSERTS__ is supplied via
cmdline when compiling project's shared libraries and executables to
treat assertions as hints (effectively similar to MS __assume
directives). It does not propagate to project's public header files.

> Said otherwise, if the user wants
> optimization hints, it should not be based on assert().

If project can invest time in refactoring their error checking code
and instructing compiler about internal invariants - that's certainly
the way to go. The only advantage of suggested approach is ease of use
(it can be deployed in couple of minutes).

> Vincent Lefèvre  - Web: 
> 100% accessible validated (X)HTML - Blog: 
> Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)