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

[Hubert Tong via cfe-commits]

+#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 --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160213/39550e41/attachment-0001.html>