This is an archive of the discontinued LLVM Phabricator instance.

[AsmWriter/APFloat] FP constant printing: Avoid usage of locale dependent snprinf
ClosedPublic

Authored by skatkov on Apr 20 2017, 1:11 AM.

Details

Summary

This should fix the bug https://bugs.llvm.org/show_bug.cgi?id=12906

To print the FP constant AsmWriter does the following:

  1. convert FP value to String (actually using snprintf function which is locale dependent).
  2. Convert String back to FP Value
  3. Compare original and got FP values. If they are not equal just dump as hex.

The problem happens on the 2nd step when APFloat does not expect group delimiter or
fraction delimiter other than period symbol and so on, which can be produced on the
first step if LLVM library is used in an environment with corresponding locale set.

To fix this issue the locale independent APFloat:toString function is used.
However it prints FP values slightly differently than snprintf does. Specifically
it suppress trailing zeros in significant, use capital E and so on.
It results in 117 test failures during make check.
To avoid this I've also updated APFloat.toString a bit to pass make check at least.

Diff Detail

Event Timeline

skatkov created this revision.Apr 20 2017, 1:11 AM
skatkov added inline comments.
lib/IR/AsmWriter.cpp
1126

Do we really need this check? To me it can be safely removed. Does anyone see the reason not to do that?

timshen edited edge metadata.Apr 20 2017, 2:07 AM

Have you considered making llvm::write_double() not to follow the user locale?

include/llvm/ADT/APFloat.h
409

Can you document TruncateZero?

lib/IR/AsmWriter.cpp
1126

Alternatively we can turn it into an assertion.

skatkov updated this revision to Diff 95919.Apr 20 2017, 3:27 AM
skatkov marked 3 inline comments as done.Apr 20 2017, 3:31 AM

Actually I considered the re-writing llvm::write_double() to be locale independent. I decided to go with solution due to:

  1. to be consistent with Lexer which also uses APFloat
  2. llvm::write_double() is used in raw_ostream which might be widely used and may be someone really want it to be locale specific.
  3. Actually re-writing llvm::write_double() will require me to just re-use printf functionality for double just only remove locale specific code. I'm not sure about licensing but it is a big piece of code to write it from scratch. However most of the code should look like APFloat::toString.

So I decided to go with this solution. Does it make sense?

rnk accepted this revision.Apr 20 2017, 11:09 AM
rnk added a subscriber: rnk.

Actually I considered the re-writing llvm::write_double() to be locale independent. I decided to go with solution due to:

  1. to be consistent with Lexer which also uses APFloat

To me, this is a really good reason to do it this way.

Looks good to me. Tim, did you have anything else?

This revision is now accepted and ready to land.Apr 20 2017, 11:09 AM
timshen accepted this revision.Apr 20 2017, 12:28 PM

LGTM!

This revision was automatically updated to reflect the committed changes.