[libcxx-commits] [PATCH] D77924: ld128 demangle: allow space for 'L' suffix.

Elliott Hughes via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Tue Aug 18 16:27:38 PDT 2020


enh added a comment.

(sorry, didn't see these comments until after i'd `git push`ed. i've failed to set up gmail filters so i see the mail i need to see without drowning in thousands of mails i don't need to see :-( )

In D77924#2186272 <https://reviews.llvm.org/D77924#2186272>, @efriedma wrote:

>> My understanding is that floating point literals are encoded using the bytes of their internal representation (https://itanium-cxx-abi.github.io/cxx-abi/abi.html#mangling.literal), and we don't know what the encoding is from the mangled name, all we can know is the builtin C++ type and the size of the encoding in bytes. We could potentially try to use that as a heuristic (e.g. 10 bytes probably means x87), but I'm not sure if that would always give the right answer (?)
>
> We could probably decode 4, 8, and 16 bytes literals as IEEE floats, and 10 bytes as x86 long double, and be right 99% of the time.  The only remotely realistic failure mode is that there's an alternative 16-byte float type on certain PowerPC variants.

yes, demangling an Android arm64 name on Linux x86 would get you a wrong answer. (though, as the other comment said, at least it won't crash now!)

your suggestion would cover everything i know of, but...

> Alternatively, we could skip decoding and dump the raw bytes; it's rare for a mangled name to contain a float literal in the first place, so most people probably won't notice.
>
>> and it would mean that we'd have to write our own floating-point printers for other representations in libcxxabi, which sounds like a pain. WDYT?
>
> LLVM's APFloat has the necessary code.  I guess you can't use that directly from libcxxabi, though?

...that sounds like a problem. (though since there are two copies of the demangler, at least _one_ of them could switch!)

also --- does the demangler even know the arch it's demangling for? i didn't see that in the bits of the code i'd looked at at least.

>> (Regardless though, I think we should land this crash-fix.)
>
> Yes.  And maybe also change the code to use snprintf, so we never end up with a buffer overflow.

as you saw, it does use snprintf, but the buffer size was too small.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77924



More information about the libcxx-commits mailing list