Patch for Bug 26283: float.h is missing mandatory C11 fp macros like DBL_DECIMAL_DIG and LDBL_DECIMAL_DIG

[Jorge Teixeira via cfe-commits]

Hubert,

You're right about the *_MIN relationships, and I fixed them on the
attached patch.

As for the enums, since there we're not even testing if the literals
are integers or fp numbers, and the Std. already reserves ranges for
implementation-specific values for some macros, it felt more natural
to simply test the boundary. The only exception would be
*_HAS_SUBNORM, for which only three values are allowed. The attached
patch implements this.

Jorge


On Sat, Feb 13, 2016 at 11:21 AM, Hubert Tong
<hubert.reinterpretcast at gmail.com> wrote:
> +#if ((FLT_MIN < DBL_MIN) || (DBL_MIN < LDBL_MIN))
> +    #error "Mandatory macros {FLT,DBL,LDBL}_MIN are invalid."
> This value again depends on the minimum exponent, and so the relationship
> being tested here is not required to hold.
> +#endif
>
> For the enumeration-like cases, perhaps it would be better to test that the
> value is one of the specific values.
>
> -- HT
>
> On Fri, Feb 12, 2016 at 11:39 PM, Jorge Teixeira
> <j.lopes.teixeira at gmail.com> wrote:
>>
>> Hi,
>>
>> I decided to strike while the iron was hot and add the remaining tests
>> for float.h.
>>
>> 1) clang was missing the C11 mandatory *_HAS_SUBNORM macros, so I
>> added them. The internal underscored versions are *_HAS_DENORM, and
>> the Std. defines only "subnormal" and "unnormalized", so there could
>> be, in theory, a discrepancy. I tried to learn more about APfloat
>> supported types (IEEEsingle,PPCDoubleDouble,etc.) and how the
>> underscored macros are generated in
>> /lib/Preprocessor/InitPreprocessor.cpp, but it was inconclusive
>> whether *_HAS_DENORM was added to mean subnormal like C11 expects, or
>> not normalized. If the former, all is good, if the latter, my patch is
>> wrong and C11 compliance is not achieved - the solution would be to
>> study all supported fp implementations and add a new macro stating
>> only the subnormal capabilities.
>>
>> 2) FLT_EVAL_METHOD was only introduced in C99, so I changed float.h
>> and float.c to reflect that.
>>
>> 3) To help ensure that all macros were tested, I reordered them in
>> float.h and float.c to match the C11 section. This added a little
>> noise to this diff, but should be a one-off thing and simplify
>> maintenance if further tests or new macros are added in the future.
>>
>> 4) The tests for the remaining macros in float.h were added. I have
>> some reservations about the ones involving floating point literals
>> (*_MAX, *_EPSILON, *_MIN, *_TRUE_MIN) due to the conversions and
>> rounding among the types. Not sure how to improve them without making
>> assumptions and/or overcomplicating the test
>>
>> (https://randomascii.wordpress.com/2012/02/25/comparing-floating-point-numbers-2012-edition/).
>>
>> 5) There were no meaningful fp changes in the Technical Corrigenda for
>> C89, so the current tests (c89,c99,c11) should suffice. Not sure if
>> gnuxx modes are affected, but I don't expect them to define
>> __STRICT_ANSI__, so all macros should be exposed and tested
>> successfully.
>>
>>
>> Cheers,
>>
>> JT
>>
>> On Fri, Feb 12, 2016 at 2:32 PM, Hubert Tong
>> <hubert.reinterpretcast at gmail.com> wrote:
>> > Committed as r260710.
>> >
>> >
>> > On Fri, Feb 12, 2016 at 9:53 AM, Hubert Tong
>> > <hubert.reinterpretcast at gmail.com> wrote:
>> >>
>> >> Thanks Jorge. I'll work on committing this today.
>> >>
>> >> -- HT
>> >>
>> >> On Fri, Feb 12, 2016 at 12:10 AM, Jorge Teixeira
>> >> <j.lopes.teixeira at gmail.com> wrote:
>> >>>
>> >>> Hubert,
>> >>>
>> >>> Thanks for the code review. Over the weekend I'll try to learn a bit
>> >>> more about using Phabricator, but for now I'll reply here, and attach
>> >>> a new patch.
>> >>>
>> >>> a) *_MANT_DIG < 1 --> *_MANT_DIG < 2
>> >>> That is a stricter check and I agree with your rationale. Done.
>> >>>
>> >>> b) _MIN_EXP --> FLT_MIN_EXP
>> >>> Done.
>> >>>
>> >>> c) Remove _MIN_EXP and _MIN_10_EXP FLT,DBL,LDBL comparisons
>> >>> Yes, as you and Richard pointed out the added mantissa bits can
>> >>> compensate for the lack of increase of the exponent.
>> >>> Already fixed in http://reviews.llvm.org/rL260639.
>> >>>
>> >>> d) *_MAX_EXP and *_MIN_EXP 2,-2 --> 1,-1
>> >>> Done.
>> >>>
>> >>> Richard, will do re: single patch for multiple files. Also, can you
>> >>> close the bug report? Even if more tests for float.h get
>> >>> added/changed, the original problem has been solved.
>> >>>
>> >>> JT
>> >>>
>> >>>
>> >>> On Thu, Feb 11, 2016 at 8:38 PM, Hubert Tong
>> >>> <hubert.reinterpretcast at gmail.com> wrote:
>> >>> > Hi Jorge,
>> >>> >
>> >>> > I responded to the initial commit with some comments here:
>> >>> > http://reviews.llvm.org/rL260577
>> >>> >
>> >>> > -- HT
>> >>> >
>> >>> > On Thu, Feb 11, 2016 at 7:53 PM, Jorge Teixeira
>> >>> > <j.lopes.teixeira at gmail.com>
>> >>> > wrote:
>> >>> >>
>> >>> >> > You'll also need to change <float.h> to only provide DECIMAL_DIG
>> >>> >> > in
>> >>> >> > C99
>> >>> >> > onwards.
>> >>> >> Done!
>> >>> >>
>> >>> >> > All of our -std versions are that standard plus applicable Defect
>> >>> >> > Reports. So -std=c89 includes TC1 and TC2, but not Amendment 1
>> >>> >> > (we
>> >>> >> > have -std=c94 for that, but the only difference from our C89 mode
>> >>> >> > is
>> >>> >> > the addition of digraphs).
>> >>> >> I'll try to find the c89 TC2 and check if anything changed
>> >>> >> regarding
>> >>> >> these macros (unlikely).
>> >>> >>
>> >>> >> > __STRICT_ANSI__ is defined if Clang has not been asked to provide
>> >>> >> > extensions (either GNU extensions, perhaps via a flag like
>> >>> >> > -std=gnu99,
>> >>> >> > or MS extensions), and is used by C library headers to determine
>> >>> >> > that
>> >>> >> > they should provide a strictly-conforming set of declarations
>> >>> >> > without
>> >>> >> > extensions.
>> >>> >> Ok, so if !defined(__STRICT__ANSI__) clang should always expose "as
>> >>> >> much as possible", including stuff from later versions of the Std.
>> >>> >> and/or eventual extensions, just as it now on float.h and float.c,
>> >>> >> right?
>> >>> >>
>> >>> >> > Testing __STDC_VERSION__ for C94 makes sense if you're trying to
>> >>> >> > detect whether Amendment 1 features should be provided.
>> >>> >> Since this will affect only digraphs, I guess there is no need (for
>> >>> >> float.h, float.c).
>> >>> >>
>> >>> >> >> 3) Lastly, can you expand (...)
>> >>> >> >
>> >>> >> > No, it does not mean that.
>> >>> >> >
>> >>> >> > For PPC64, long double is (sometimes) modeled as a pair of
>> >>> >> > doubles.
>> >>> >> > Under that model, the smallest normalized value for long double
>> >>> >> > is
>> >>> >> > actually larger than the smallest normalized value for double
>> >>> >> > (remember that for a normalized value with exponent E, all
>> >>> >> > numbers
>> >>> >> > of
>> >>> >> > the form 1.XXXXX * 2^E, with the right number of mantissa digits,
>> >>> >> > are
>> >>> >> > exactly representable, so increasing the number of mantissa bits
>> >>> >> > without changing the number of exponent bits increases the
>> >>> >> > magnitude
>> >>> >> > of the smallest normalized positive number).
>> >>> >> >
>> >>> >> > The set of values of long double in this model *is* a superset of
>> >>> >> > the
>> >>> >> > set of values of double.
>> >>> >> >
>> >>> >> I see now, and removed the bogus tests. The patch should now test
>> >>> >> cleanly unless something needs DECIMAL_DIG but did not set the
>> >>> >> appropriate std. level, or defined __STRICT__ANSI__.
>> >>> >>
>> >>> >> Thanks for the learning experience,
>> >>> >>
>> >>> >> JT
>> >>> >>
>> >>> >>
>> >>> >>
>> >>> >> >> From /test/Preprocessor/init.cpp:
>> >>> >> >> // PPC64:#define __DBL_MIN_EXP__ (-1021)
>> >>> >> >> // PPC64:#define __FLT_MIN_EXP__ (-125)
>> >>> >> >> // PPC64:#define __LDBL_MIN_EXP__ (-968)
>> >>> >> >>
>> >>> >> >> This issue happened before
>> >>> >> >>
>> >>> >> >>
>> >>> >> >> (https://lists.gnu.org/archive/html/bug-gnulib/2011-08/msg00262.html,
>> >>> >> >> http://www.openwall.com/lists/musl/2013/11/15/1), but all it
>> >>> >> >> means
>> >>> >> >> is
>> >>> >> >> that ppc64 is not compliant with C without soft-float. The test
>> >>> >> >> is
>> >>> >> >> valid and should stay, and if someone tries to compile for ppc64
>> >>> >> >> in
>> >>> >> >> c89, c99 or c11 modes, clang should 1) use soft float (bad
>> >>> >> >> idea),
>> >>> >> >> 2)
>> >>> >> >> issue a diagnostic saying that that arch cannot meet the desired
>> >>> >> >> C
>> >>> >> >> standard without a big performance penalty - the diag should be
>> >>> >> >> suppressible with some special cmd line argument.
>> >>> >> >> Thus, I added the tests back and the FAIL for PPC64 for the time
>> >>> >> >> being, with a comment. If you know of a way to skip only the
>> >>> >> >> specific
>> >>> >> >> *_MIN_EXP and *_MIN_10_EXP tests, please add it, because there
>> >>> >> >> might
>> >>> >> >> be more similar cases in the future.
>> >>> >> >>
>> >>> >> >> JT
>> >>> >> >>
>> >>> >> >> On Thu, Feb 11, 2016 at 3:04 PM, Richard Smith
>> >>> >> >> <richard at metafoo.co.uk>
>> >>> >> >> wrote:
>> >>> >> >>> Thanks, I modified the test to also test C89 and C99 modes and
>> >>> >> >>> committed this as r260577.
>> >>> >> >>>
>> >>> >> >>> On Thu, Feb 11, 2016 at 11:29 AM, Jorge Teixeira
>> >>> >> >>> <j.lopes.teixeira at gmail.com> wrote:
>> >>> >> >>>> Here is a revised test, which I renamed to c11-5_2_4_2_2p11.c
>> >>> >> >>>> instead
>> >>> >> >>>> of float.c because I am only checking a subset of what the
>> >>> >> >>>> standard
>> >>> >> >>>> mandates for float.h, and because there were similar
>> >>> >> >>>> precedents,
>> >>> >> >>>> like
>> >>> >> >>>> test/Preprocessor/c99-*.c. Feel free to override, though.
>> >>> >> >>>
>> >>> >> >>> test/Preprocessor/c99-* are an aberration. The goal would be
>> >>> >> >>> that
>> >>> >> >>> this
>> >>> >> >>> test grows to cover all of the parts of float.h that we define,
>> >>> >> >>> so
>> >>> >> >>> float.c seems like the appropriate name for it.
>> >>> >> >>>
>> >>> >> >>>> The first part checks for basic compliance with the referred
>> >>> >> >>>> C11
>> >>> >> >>>> paragraph, the second for internal consistency between the
>> >>> >> >>>> underscored
>> >>> >> >>>> and exposed versions of the macros.
>> >>> >> >>>> No attempt was made to support C99 or C89.
>> >>> >> >>>>
>> >>> >> >>>> I am not very clear on the proper use of the whole lit.py /
>> >>> >> >>>> RUN
>> >>> >> >>>> framework, so someone should really confirm if what I wrote is
>> >>> >> >>>> correct. The goal was to test both hosted and freestanding
>> >>> >> >>>> implementations with C11, and expect no diagnostics from
>> >>> >> >>>> either.
>> >>> >> >>>
>> >>> >> >>> We generally avoid testing hosted mode, because we don't want
>> >>> >> >>> the
>> >>> >> >>> success of our tests to depend on the libc installed on the
>> >>> >> >>> host
>> >>> >> >>> system.
>> >>> >> >>>
>> >>> >> >>>> Thanks for the help,
>> >>> >> >>>>
>> >>> >> >>>> JT
>> >>> >> >>>>
>> >>> >> >>>> On Tue, Feb 9, 2016 at 5:56 PM, Richard Smith
>> >>> >> >>>> <richard at metafoo.co.uk>
>> >>> >> >>>> wrote:
>> >>> >> >>>>> On Tue, Feb 9, 2016 at 2:43 PM, Jorge Teixeira
>> >>> >> >>>>> <j.lopes.teixeira at gmail.com> wrote:
>> >>> >> >>>>>> Richard,
>> >>> >> >>>>>>
>> >>> >> >>>>>> Can you be more specific?
>> >>> >> >>>>>>
>> >>> >> >>>>>> I assume you mean something like my newly attached .h file
>> >>> >> >>>>>> that
>> >>> >> >>>>>> tests
>> >>> >> >>>>>> very basic implementation compliance (i.e., it's required,
>> >>> >> >>>>>> but
>> >>> >> >>>>>> not
>> >>> >> >>>>>> sufficient), but I would need a bit more guidance about the
>> >>> >> >>>>>> structure
>> >>> >> >>>>>> of the file, how to perform the tests, and where to exactly
>> >>> >> >>>>>> place
>> >>> >> >>>>>> and
>> >>> >> >>>>>> name the file within test/Headers.
>> >>> >> >>>>>>
>> >>> >> >>>>>> I some sort of template exists, or if someone else takes
>> >>> >> >>>>>> point
>> >>> >> >>>>>> and
>> >>> >> >>>>>> makes it, I can "port" the attached p11 test cases. I am
>> >>> >> >>>>>> unsure
>> >>> >> >>>>>> of
>> >>> >> >>>>>> how
>> >>> >> >>>>>> to perform a more normative compliance - for example, to
>> >>> >> >>>>>> assert
>> >>> >> >>>>>> that
>> >>> >> >>>>>> LDBL_DECIMAL_DIG is 21 on x86-64 and that indeed those many
>> >>> >> >>>>>> digits
>> >>> >> >>>>>> are
>> >>> >> >>>>>> guaranteed to be correct, etc. This is probably not possible
>> >>> >> >>>>>> /
>> >>> >> >>>>>> does
>> >>> >> >>>>>> not make sense.
>> >>> >> >>>>>
>> >>> >> >>>>> That looks like a decent basic test for this. The test should
>> >>> >> >>>>> be
>> >>> >> >>>>> named
>> >>> >> >>>>> something like test/Headers/float.c, and needs to contain a
>> >>> >> >>>>> "RUN:"
>> >>> >> >>>>> line so that the test runner infrastructure knows how to run
>> >>> >> >>>>> it.
>> >>> >> >>>>> You
>> >>> >> >>>>> can look at test/Header/limits.cpp for an example of how this
>> >>> >> >>>>> works.
>> >>> >> >>>>>
>> >>> >> >>>>> We already have platform-specific tests that
>> >>> >> >>>>> __LDBL_DECIMAL_DIG__ is
>> >>> >> >>>>> the right value, so you could test the values are correct by
>> >>> >> >>>>> checking
>> >>> >> >>>>> that LDBL_DECIMAL_DIG == __LDBL_DECIMAL_DIG__.
>> >>> >> >>>>>
>> >>> >> >>>>>> JT
>> >>> >> >>>>>>
>> >>> >> >>>>>> On Tue, Feb 9, 2016 at 3:58 PM, Richard Smith
>> >>> >> >>>>>> <richard at metafoo.co.uk> wrote:
>> >>> >> >>>>>>> Patch looks good. Please also add a testcase to
>> >>> >> >>>>>>> test/Headers.
>> >>> >> >>>>>>>
>> >>> >> >>>>>>> On Tue, Feb 9, 2016 at 12:08 PM, Hubert Tong via
>> >>> >> >>>>>>> cfe-commits
>> >>> >> >>>>>>> <cfe-commits at lists.llvm.org> wrote:
>> >>> >> >>>>>>>> I see no immediate issue with this patch, but I am not one
>> >>> >> >>>>>>>> of
>> >>> >> >>>>>>>> the
>> >>> >> >>>>>>>> usual
>> >>> >> >>>>>>>> reviewers for this part of the code base.
>> >>> >> >>>>>>>>
>> >>> >> >>>>>>>> -- HT
>> >>> >> >>>>>>>>
>> >>> >> >>>>>>>>
>> >>> >> >>>>>>>> On Tue, Feb 9, 2016 at 2:56 PM, Jorge Teixeira
>> >>> >> >>>>>>>> <j.lopes.teixeira at gmail.com>
>> >>> >> >>>>>>>> wrote:
>> >>> >> >>>>>>>>>
>> >>> >> >>>>>>>>> Thanks Hubert. Somehow I omitted that prefix when typing
>> >>> >> >>>>>>>>> the
>> >>> >> >>>>>>>>> macros,
>> >>> >> >>>>>>>>> and I did not noticed it when I was testing because on my
>> >>> >> >>>>>>>>> arch
>> >>> >> >>>>>>>>> DECIMAL_DIG is defined to be the LDBL version...
>> >>> >> >>>>>>>>>
>> >>> >> >>>>>>>>> Updated patch is attached.
>> >>> >> >>>>>>>>>
>> >>> >> >>>>>>>>> JT
>> >>> >> >>>>>>>>>
>> >>> >> >>>>>>>>> On Tue, Feb 9, 2016 at 1:41 PM, Hubert Tong
>> >>> >> >>>>>>>>> <hubert.reinterpretcast at gmail.com> wrote:
>> >>> >> >>>>>>>>> > There is a __LDBL_DECIMAL_DIG__ predefined macro.
>> >>> >> >>>>>>>>> > __DECIMAL_DIG__ will
>> >>> >> >>>>>>>>> > not
>> >>> >> >>>>>>>>> > always be the same as __LDBL_DECIMAL_DIG__.
>> >>> >> >>>>>>>>> >
>> >>> >> >>>>>>>>> > -- HT
>> >>> >> >>>>>>>>> >
>> >>> >> >>>>>>>>> > On Mon, Feb 8, 2016 at 11:26 PM, Jorge Teixeira via
>> >>> >> >>>>>>>>> > cfe-commits
>> >>> >> >>>>>>>>> > <cfe-commits at lists.llvm.org> wrote:
>> >>> >> >>>>>>>>> >>
>> >>> >> >>>>>>>>> >> Hi, I filed the bug
>> >>> >> >>>>>>>>> >> (https://llvm.org/bugs/show_bug.cgi?id=26283) some
>> >>> >> >>>>>>>>> >> time ago and nobody picked it up, so here is a trivial
>> >>> >> >>>>>>>>> >> patch
>> >>> >> >>>>>>>>> >> exposing
>> >>> >> >>>>>>>>> >> the missing macros, that to the best of my ability
>> >>> >> >>>>>>>>> >> were
>> >>> >> >>>>>>>>> >> already
>> >>> >> >>>>>>>>> >> present as the internal underscored versions.
>> >>> >> >>>>>>>>> >>
>> >>> >> >>>>>>>>> >> Perhaps a more general bug about C11 floating point
>> >>> >> >>>>>>>>> >> (lack
>> >>> >> >>>>>>>>> >> of)
>> >>> >> >>>>>>>>> >> conformance should be filed, so that some form of unit
>> >>> >> >>>>>>>>> >> test/macro
>> >>> >> >>>>>>>>> >> validation could be worked on, but this patch does
>> >>> >> >>>>>>>>> >> scratch my
>> >>> >> >>>>>>>>> >> current
>> >>> >> >>>>>>>>> >> itch.
>> >>> >> >>>>>>>>> >>
>> >>> >> >>>>>>>>> >> Successfully tested on x86-64 Xubuntu 14.04 with clang
>> >>> >> >>>>>>>>> >> 3.8
>> >>> >> >>>>>>>>> >> from the
>> >>> >> >>>>>>>>> >> ppa, patched with the attached diff.
>> >>> >> >>>>>>>>> >>
>> >>> >> >>>>>>>>> >> First contribution, so feel free to suggest
>> >>> >> >>>>>>>>> >> improvements
>> >>> >> >>>>>>>>> >> or
>> >>> >> >>>>>>>>> >> point to
>> >>> >> >>>>>>>>> >> more detailed step-by-step instructions/guidelines.
>> >>> >> >>>>>>>>> >>
>> >>> >> >>>>>>>>> >> Cheers,
>> >>> >> >>>>>>>>> >>
>> >>> >> >>>>>>>>> >> JT
>> >>> >> >>>>>>>>> >>
>> >>> >> >>>>>>>>> >> _______________________________________________
>> >>> >> >>>>>>>>> >> cfe-commits mailing list
>> >>> >> >>>>>>>>> >> cfe-commits at lists.llvm.org
>> >>> >> >>>>>>>>> >>
>> >>> >> >>>>>>>>> >>
>> >>> >> >>>>>>>>> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>> >>> >> >>>>>>>>> >>
>> >>> >> >>>>>>>>> >
>> >>> >> >>>>>>>>
>> >>> >> >>>>>>>>
>> >>> >> >>>>>>>>
>> >>> >> >>>>>>>> _______________________________________________
>> >>> >> >>>>>>>> cfe-commits mailing list
>> >>> >> >>>>>>>> cfe-commits at lists.llvm.org
>> >>> >> >>>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>> >>> >> >>>>>>>>
>> >>> >
>> >>> >
>> >>
>> >>
>> >
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: float.patch
Type: text/x-patch
Size: 9262 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160213/4f209626/attachment-0001.bin>