[PATCH] D45418: [SimplifyLibcalls] Atoi, strtol replacements

[Dávid Bolvanský via Phabricator via llvm-commits]

xbolva00 added a comment.

Is patch now ok for you, @efriedma ?



================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:1697
+  if (ConstantInt *CInt = dyn_cast<ConstantInt>(CI->getArgOperand(2)))
+    return ConstantInt::get(CI->getType(), strtol(Str.str().c_str(), nullptr,
+                                                  CInt->getSExtValue()));
----------------
xbolva00 wrote:
> xbolva00 wrote:
> > efriedma wrote:
> > > lebedev.ri wrote:
> > > > lebedev.ri wrote:
> > > > > `base` is `i32`, but `strtol` returns `long int` (`i64`?), so i don't think using `CI->getType()` is correct.
> > > > > 
> > > > Oh, `CI` is `CallInst`, not `ConstantInt`, never mind then.
> > > This is missing some checks.  In particular, what happens if the host's "long" is a different width from the target's "long"? What happens if strtol sets errno?  What happens if the target program is using a locale where isspace() doesn't match the C locale's isspace()?
> > Ah yes. So I will have to close this patch since we cant optimize this calls.
> I dont know about that thing with locale. I dont think we have to care about this weirdness.
Errno can be checked, can be fixed in this patch.

But why we should check target host vs host long ? 


================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:1697
+  if (ConstantInt *CInt = dyn_cast<ConstantInt>(CI->getArgOperand(2)))
+    return ConstantInt::get(CI->getType(), strtol(Str.str().c_str(), nullptr,
+                                                  CInt->getSExtValue()));
----------------
efriedma wrote:
> xbolva00 wrote:
> > xbolva00 wrote:
> > > xbolva00 wrote:
> > > > efriedma wrote:
> > > > > lebedev.ri wrote:
> > > > > > lebedev.ri wrote:
> > > > > > > `base` is `i32`, but `strtol` returns `long int` (`i64`?), so i don't think using `CI->getType()` is correct.
> > > > > > > 
> > > > > > Oh, `CI` is `CallInst`, not `ConstantInt`, never mind then.
> > > > > This is missing some checks.  In particular, what happens if the host's "long" is a different width from the target's "long"? What happens if strtol sets errno?  What happens if the target program is using a locale where isspace() doesn't match the C locale's isspace()?
> > > > Ah yes. So I will have to close this patch since we cant optimize this calls.
> > > I dont know about that thing with locale. I dont think we have to care about this weirdness.
> > Errno can be checked, can be fixed in this patch.
> > 
> > But why we should check target host vs host long ? 
> isspace() in all locales should return true for all the characters where it returns true in the C locale; it's just a question of whether there are some additional characters which also count as spaces.  So I think you'll get the right behavior if you refuse to fold in cases where strtol would fail in the C locale.
Updated patch. Check for "C" locale added.


================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:1685
+
+  return ConstantInt::get(CI->getType(), atoi(Str.str().c_str()));
+}
----------------
efriedma wrote:
> I'm worried about host behavior leaking through here; specifically, the "undefined" values for overflow might vary. Could you use some function with more predictable behavior here?  (Maybe something based on llvm::consumeSignedInteger()?)
We should call that function when we can call strtol directly?


https://reviews.llvm.org/D45418