Page MenuHomePhabricator

[SimplifyLibcalls] Replace locked IO with unlocked IO
ClosedPublic

Authored by xbolva00 on Apr 17 2018, 2:05 PM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
xbolva00 updated this revision to Diff 142980.Apr 18 2018, 12:22 PM
xbolva00 added a comment.EditedApr 18 2018, 12:29 PM

NoCapture attributes check now works, I forgot about inferLibFuncAttributes..

But I still get a issue like:

define i32 @aa() #0 {
entry:

%call = call i32 @aac()
ret i32 %call

}

; Function Attrs: noinline nounwind uwtable
define internal i32 @aac() #0 {
entry:

%getchar_unlocked = call i32 @getchar_unlocked()
ret i32 %getchar_unlocked

}

Any idea how to fix it, @efriedma ?

I don't think there's any practical way to prove that no other thread will write to stdout.

xbolva00 added a comment.EditedApr 18 2018, 1:52 PM

I don't think there's any practical way to prove that no other thread will write to stdout.

I can check if non static functions call a certain static function.

It doesn't matter what code is in the module which is currently being processed; another module could do __attribute((constructor)) void f() { pthread_t t; pthread_create(&t, 0, puts, "foo"); } or something.

xbolva00 updated this revision to Diff 142997.EditedApr 18 2018, 2:22 PM

It doesn't matter what code is in the module which is currently being processed; another module could do __attribute((constructor)) void f() { pthread_t t; pthread_create(&t, 0, puts, "foo"); } or something.

Then, this patch is dead, right? Or?

If there are no external functions that could call static ones which contain IO calls.. we are still safe to do this transformation, nope?

It's still possible to optimize your capt_test() example.

xbolva00 updated this revision to Diff 143007.Apr 18 2018, 3:00 PM

It's still possible to optimize your capt_test() example.

Yes, I updated this patch.

xbolva00 updated this revision to Diff 143020.Apr 18 2018, 3:47 PM
xbolva00 added a comment.EditedApr 19 2018, 7:00 AM

@efriedma, May I add tests for "capture detection" only for one IO call, e.g. fgetc? Since all others depends on the "isReplaceableWithUnlockedVariant" anyway..

xbolva00 updated this revision to Diff 143094.Apr 19 2018, 7:42 AM

Which additional tests you would like to see?

efriedma added inline comments.Apr 20 2018, 12:00 PM
lib/Analysis/TargetLibraryInfo.cpp
328 ↗(On Diff #143094)

We probably want to whitelist which operating systems these are available on, rather than blacklist; most of these aren't part of any standard.

And actually, thinking about it a bit more, the "non-standard" part has other problems; a user could define their own function named fputc_unlocked, which does something different from the GNU functions. (Nothing else in the current BuildLibCalls.h hits this particular problem because those are all reserved names.) But I'm not sure if that's likely to be a problem in practice.

lib/Transforms/Utils/SimplifyLibCalls.cpp
126 ↗(On Diff #143094)

This check doesn't buy you anything.

135 ↗(On Diff #143094)

This is incomplete; you also need to check that the instruction which produces the FILE* is a call to fopen, or the capture analysis won't do anything useful. Consider:

FILE* foo() { return stdout; }
void bar() { fputc('x', foo()); }

Also, iterating over the operands like this is weird; probably want to explicitly pass in which operand is the FILE*.

xbolva00 updated this revision to Diff 143366.Apr 20 2018, 12:46 PM

Fixed things .

xbolva00 marked 3 inline comments as done.Apr 20 2018, 12:47 PM
xbolva00 updated this revision to Diff 143368.Apr 20 2018, 12:50 PM

Updated tests.

xbolva00 updated this revision to Diff 143377.Apr 20 2018, 1:34 PM

Better tests

xbolva00 updated this revision to Diff 143454.Apr 21 2018, 9:02 AM
xbolva00 edited the summary of this revision. (Show Details)
This comment was removed by xbolva00.
xbolva00 updated this revision to Diff 144090.Apr 26 2018, 2:58 AM

Updated tests
Renamed helper function to isLocallyOpenedFile.

xbolva00 updated this revision to Diff 144196.Apr 26 2018, 2:28 PM
xbolva00 updated this revision to Diff 144204.Apr 26 2018, 2:43 PM
xbolva00 updated this revision to Diff 144206.Apr 26 2018, 2:51 PM
This revision was not accepted when it landed; it landed in state Needs Review.Apr 26 2018, 3:35 PM
This revision was automatically updated to reflect the committed changes.

Was this actually reviewed?
The differential is not marked as accepted.

xbolva00 added a comment.EditedApr 26 2018, 3:53 PM

Was this actually reviewed?
The differential is not marked as accepted.

Notes were fixes and no futher comments were given... even if I pinged it.

I will check how to revert via arc so..

Edit: can you revert it? arc revert doesnt work with svn only.

This seems to have broken the sanitizer bots: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/17377.

I will revert for now.

morehouse reopened this revision.Apr 26 2018, 6:52 PM

Reverted in r331011.

Was this actually reviewed?
The differential is not marked as accepted.

Notes were fixes and no futher comments were given... even if I pinged it.

That is not how this works though.

https://llvm.org/docs/Phabricator.html#committing-a-change

Once a patch has been reviewed and approved on Phabricator it can then be committed to trunk.

I will check how to revert via arc so..

Edit: can you revert it? arc revert doesnt work with svn only.

xbolva00 updated this revision to Diff 144292.Apr 27 2018, 12:59 AM

Fixed build problems

xbolva00 added a comment.EditedApr 27 2018, 1:02 AM

@efriedma , @lebedev.ri:

Fixed and tested:
ThinLTO/X86/tli-nobuiltin.ll
Transforms/InstCombine/fprintf-1.ll
Transforms/InstCombine/fputs-1.ll

So it should not break the build.

I updated TargetLibraryInfoTest.cpp to fix other problem too..

but I got some problems when running ninja check-all
unittests/Transforms/Utils/CMakeFiles/UtilsTests.dir/IntegerDivision.cpp.o: In function `(anonymous namespace)::IntegerDivision_SRem64_Test::TestBody()':
IntegerDivision.cpp:(.text._ZN12_GLOBAL__N_127IntegerDivision_SRem64_Test8TestBodyEv+0x39c): undefined reference to `llvm::expandRemainder(llvm::BinaryOperator*)'

So please somebody verify this new revision too. Thanks.

xbolva00 updated this revision to Diff 144293.Apr 27 2018, 1:03 AM

annotation.ll test needs to be too, I will check it.

Any other things to do?

xbolva00 updated this revision to Diff 144437.Apr 28 2018, 1:59 AM
This comment was removed by xbolva00.

Hi @lebedev.ri ,
Can you recommend me some person who is able to review this patch?

I ran test-suite, no breakages. Older notes to the patch was fixed. I think it is good to go.

Sorry about the delay, I got busy with other things.

lib/Analysis/TargetLibraryInfo.cpp
486 ↗(On Diff #144437)

You probably also need to check T.isAndroid(), like the code above this. (I haven't actually checked what bionic provides, but I'm guessing it doesn't have all of these.)

lib/Transforms/Utils/SimplifyLibCalls.cpp
135 ↗(On Diff #144437)

I think you also need to check if the call is marked nobuiltin.

test/Transforms/InstCombine/unlocked-stdio.ll
4 ↗(On Diff #144437)

It would be nice to have at least some tests which call fclose (since otherwise you're leaking the file handle).

Needs some negative tests to make sure the capture checking is working correctly.

Sorry about the delay, I got busy with other things.

Ok, no problem. I will fix your notes :)

xbolva00 added inline comments.May 1 2018, 12:17 AM
lib/Analysis/TargetLibraryInfo.cpp
486 ↗(On Diff #144437)

They will be available since Android P, so I will enable this for Android P and higher

https://android.googlesource.com/platform/bionic/+/master/docs/status.md

xbolva00 updated this revision to Diff 144678.May 1 2018, 12:44 AM

Fixed things.

xbolva00 marked 3 inline comments as done.May 1 2018, 12:44 AM
xbolva00 updated this revision to Diff 144796.May 1 2018, 3:02 PM
This comment was removed by xbolva00.
xbolva00 updated this revision to Diff 144798.May 1 2018, 3:03 PM

Check if the call is NoBuiltin

xbolva00 updated this revision to Diff 144799.EditedMay 1 2018, 3:07 PM

Removed redundant check

lib/Transforms/Utils/SimplifyLibCalls.cpp
135 ↗(On Diff #144437)

I added check but I now see that optimizeCall checks for this also.

Edit: removed nobuiltin check

friendly ping :) we are close with this patch to get it done

The code looks fine. But I'm a bit worried this could break in some cases. It's possible a user could define a function named fputc_unlocked, so we wouldn't accept a valid C program. And it's possible this could break some runtime instrumentation tools which haven't implemented the *_unlocked functions. Or someone could use some obscure C library on Linux which doesn't have all the glibc extensions and get surprised by a link error.

So I'm not sure what to do here; maybe get a second opinion on llvmdev?

xbolva00 added a comment.EditedMay 8 2018, 4:50 PM

The code looks fine. But I'm a bit worried this could break in some cases. It's possible a user could define a function named fputc_unlocked, so we wouldn't accept a valid C program. And it's possible this could break some runtime instrumentation tools which haven't implemented the *_unlocked functions. Or someone could use some obscure C library on Linux which doesn't have all the glibc extensions and get surprised by a link error.

So I'm not sure what to do here; maybe get a second opinion on llvmdev?

We could check if e.g. fputc_unlocked is user defined in the current module, no?

Instrumentation tools are fixable, they would have to cover more functions but it is not a useless work - their analysis will just improve. I believe they would have to add some new cases to switch as a solution, etc.

Obscure C library: I believe if such C library, nobody would use Clang 7 on such system.

It would live in the trunk and it could be reverted in 7.0.1 if any major problems reported.

xbolva00 added inline comments.May 8 2018, 5:55 PM
lib/Analysis/TargetLibraryInfo.cpp
485 ↗(On Diff #144799)

if (T.isGNUEnvironment()) ?

So as a solution

if (isLinux && isGNU)?

xbolva00 updated this revision to Diff 146040.May 9 2018, 5:36 PM

Handle GNU extensions under isGNUEnvironment (same way as we do for sincos)

xbolva00 added inline comments.May 9 2018, 5:38 PM
lib/Analysis/TargetLibraryInfo.cpp
485 ↗(On Diff #144799)
rja accepted this revision.May 12 2018, 5:15 AM
rja added a subscriber: rja.

LGTM.

We should merge this patch to see the impact of this patch on real code bases. We can still revert it if too many problems.

This revision is now accepted and ready to land.May 12 2018, 5:15 AM
This revision was automatically updated to reflect the committed changes.
In D45736#1096871, @rja wrote:

LGTM.

We should merge this patch to see the impact of this patch on real code bases. We can still revert it if too many problems.

We're definitely seeing problems here and I'm not sure if it's safe in general. Also, before committing we'd prefer that actual committers review patches - and I'm not seeing that you're an active committer to the project. If so, could you let us know who you are?

In particular, for this patch I think it should have gone to llvm-dev first as Eli suggested.

Thanks.

-eric

xbolva00 added a comment.EditedMay 17 2018, 2:30 PM

! In D45736#1103697, @echristo wrote:
We're definitely seeing problems here and I'm not sure if it's safe in general.

...

-eric

There was a llvm-dev thread. but only me, Eli and Han participated. There was a discussion that Linux && GNU check would be enough to be sure we have these unlocked functions.

Can you write here your problems?

The transformation logic is safe I think, we are sure via capture tracker that if fileptr cannot be captured & fileptr comes from fopen, we can use unlocked variants.

But I cannot do tests everywhere with every glibc versions, every XY system :)

And If almost nobody write comments (btw, I would like say thanks to Eli for advices not only in this patch) during review (and he wrote the code is OK), llvm-dev thread, we cannot do this transformation better, safer (?) ...

! In D45736#1103697, @echristo wrote:
We're definitely seeing problems here and I'm not sure if it's safe in general.

...

-eric

There was a llvm-dev thread. but only me, Eli and Han participated. There was a discussion that Linux && GNU check would be enough to be sure we have these unlocked functions.

Can you write here your problems?

The transformation logic is safe I think, we are sure via capture tracker that if fileptr cannot be captured & fileptr comes from fopen, we can use unlocked variants.

But I cannot do tests everywhere with every glibc versions, every XY system :)

And If almost nobody write comments (btw, I would like say thanks to Eli for advices not only in this patch) during review (and he wrote the code is OK), llvm-dev thread, we cannot do this transformation better, safer (?) ...

Ben's fix and comment on the patch were the problems I was seeing. More so that no actual committer to the project approved - no one I've spoken to knows who rja is.

xbolva00 added a comment.EditedMay 17 2018, 2:54 PM

So please revert it and if possible, write here problems what this caused. Maybe it is possible to fix it.

This comment was removed by xbolva00.

Not sure about benchmarks, if there are any for C FILE IO. But in my tests and programs I see small improvements - yes, they read and write a lot from / to files.

Would you like to remove it?
... or make it off by default and provide option to turn this on if anybody wants?

This comment was removed by xbolva00.