[libcxx-commits] [PATCH] D74163: [demangler] Fix the parsing of long double literals for PowerPC and S390

[Xing Xue via Phabricator via libcxx-commits]

xingxue added a comment.

In D74163#1891408 <https://reviews.llvm.org/D74163#1891408>, @uweigand wrote:

> In D74163#1891378 <https://reviews.llvm.org/D74163#1891378>, @xingxue wrote:
>
> > > I'm still wondering about Intel.  Can there ever be a literal encoded using 'g' on Intel?  If yes, then treating it as "long double" would still be wrong, because 'g' encodes IEEE128 (__float128), while "long double" is the Intel extended (80-bit) format, right?
> > > 
> > > On the other hand, if 'g' encoded literals can never happen on Intel (or other platforms), maybe it would be better to have the code handling 'g' within a #ifdef section only active on powerpc and s390?
> >
> > For X86, 'e' is used for 80-bit `long double` and 'g' is used for 128-bit `long double`.  The following is the code in Clang.
> >
> >   clang/lib/Basic/Targets/X86.h
> >   ....
> >   const char *getLongDoubleMangling() const override {
> >     return LongDoubleFormat == &llvm::APFloat::IEEEquad() ? "g" : "e";
> >   }
> >   ...
> >    
>
>
> But then this patch must be incorrect, given that it does
>
>   return getDerived().template parseFloatingLiteral<long double>();
>   
>
> for **both** the 'e' and 'g' cases on Intel.    Now I guess it depends on how the file is compiled (with -mlong-double-80 or -mlong-double-128), but either way, one of the cases will be handled incorrectly.




In D74163#1891476 <https://reviews.llvm.org/D74163#1891476>, @hubert.reinterpretcast wrote:

> In D74163#1891408 <https://reviews.llvm.org/D74163#1891408>, @uweigand wrote:
>
> > But then this patch must be incorrect, given that it does
> >
> >   return getDerived().template parseFloatingLiteral<long double>();
> >   
> >
> > for **both** the 'e' and 'g' cases on Intel.    Now I guess it depends on how the file is compiled (with -mlong-double-80 or -mlong-double-128), but either way, one of the cases will be handled incorrectly.
>
>
> Agreed. The addition of the 'g' handling for cases other than the specific platforms mentioned by the title for the patch is outside the intended scope and should probably be avoided.


Agreed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74163/new/

https://reviews.llvm.org/D74163