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

[Hubert Tong via Phabricator via libcxx-commits]

hubert.reinterpretcast added a comment.
Herald added a project: libc++abi.
Herald added a reviewer: libc++abi.

It seems demangling `g` as `__float128` (and not trying to interpret its value) is to be expected. Setting up `e` to handle 64-bit long double should be sufficient. I suggest to remove changing the handling of `g` from this patch.



================
Comment at: libcxxabi/src/cxa_demangle.cpp:14
+#error "Demangler must be built in long double 128 mode on S390."
+#endif
+
----------------
None of these restrictions are necessary if we stick to the established handling of `g`, which prints the raw byte string in hex. Yes, it is unfortunate that it displays as `__float128` on Power, especially since the GCC compiler's `__float128` on Power (requiring `-mfloat128`) mangles as something else. Dealing with that is another can of worms that might need coordination with the GCC folks.


================
Comment at: libcxxabi/src/demangle/ItaniumDemangle.h:3793
+  //                ::= g    # long double, __float128
+#if defined(__powerpc__) || defined(__s390__)
+  case 'g':
----------------
I do not believe this change is necessary at this time. On `powerpc64le-linux-gnu` even with `-mlong-double-128`,
`_Z1fIiEvP1AIXszplLg00000000000000004000000000000000EcvT__EEE`
demangles using
`__cxa_demangle` picked up using `-lsupc++`
as
`void f<int>(A<sizeof (((__float128)[00000000000000004000000000000000])+((int)()))>*)`.

Similarly, the same with
`_Z1fIiEvP1AIXszplLg40000000000000000000000000000000EcvT__EEE`
on `s390x-linux-gnu` gives
`void f<int>(A<sizeof (((__float128)[40000000000000000000000000000000])+((int)()))>*)`.



================
Comment at: libcxxabi/src/demangle/ItaniumDemangle.h:4236
+    return getDerived().template parseFloatingLiteral<double>();
+  case 'g':
+    ++First;
----------------
I'm going to suggest leaving `g` alone.


================
Comment at: libcxxabi/src/demangle/ItaniumDemangle.h:5185
 #if defined(__mips__) && defined(__mips_n64) || defined(__aarch64__) || \
-    defined(__wasm__)
+    defined(__wasm__) || defined(__powerpc__) || defined(__s390__)
     static const size_t mangled_size = 32;
----------------
If we aren't changing `g` to go here, then we don't need the change here.


================
Comment at: libcxxabi/src/demangle/ItaniumDemangle.h:5192
 #endif
     static const size_t max_demangled_size = 40;
     static constexpr const char *spec = "%LaL";
----------------
This is not going to be long enough for IBM double-double, which can have a large gap between the high and the low doubles.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74163