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]

Committed as r260710 <http://reviews.llvm.org/rL260710>.

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/20160212/d2755377/attachment-0001.html>