Page MenuHomePhabricator

Don't modify LibFuncs in DeadArgumentElimination or ArgumentPromotion
AcceptedPublic

Authored by jgravelle-google on Jun 20 2018, 4:19 PM.

Details

Reviewers
dschuff
Summary

When performing IPO, it's possible to modify function signatures to simplify things. There are also passes that make assumptions about LibFunc signatures, notably DeadStoreElimination, which crashes when strcpy, strncpy, strcat, or strncat have their arugments removed.
When including libc function definitions and running IPO, LibFunc signatures can be simplified, which can invalidate those assumptions. This patch adds checks to passes that modify signatures to exclude LibFuncs.

Diff Detail

Event Timeline

dschuff accepted this revision.Jun 20 2018, 5:18 PM

A nit about the commit message: it's not just LTO passes that make assumptions about known library functions, lots of passes do. Also I might be just a little bit more explicit than "when statically linking libc" and make it "when including libc in LTO"
Otherwise LGTM

lib/Transforms/IPO/DeadArgumentElimination.cpp
83

I guess this isn't really a DummyMAM anymore if we are registering analyses on it?

This revision is now accepted and ready to land.Jun 20 2018, 5:18 PM

A nit about the commit message: it's not just LTO passes that make assumptions about known library functions, lots of passes do. Also I might be just a little bit more explicit than "when statically linking libc" and make it "when including libc in LTO"
Otherwise LGTM

Is this an issue with LTO specifically (link time optimization) or just IPO passes in general? The test case doesn't seem to involve LTO.

Probably just IPO then. It's still not WebAssembly specific.

Is there a good place to put the testcase? I guess test/Other?

lib/Transforms/IPO/DeadArgumentElimination.cpp
83

Not sure. I can't find any non-PassManager, non-test uses of registerPass other than things like https://github.com/llvm-mirror/llvm/blob/baa9ccee6421c045f639b492189da86b7e2a32dd/lib/Transforms/IPO/GlobalDCE.cpp#L58 , which every callsite I can find calls FunctionAnalysisManagerModuleProxy(DummyFAM), which probably still counts as being a Dummy.
Ideally here I'd like to register this pass as requiring TargetLibraryInfo and reusing a possibly-existing analysis, but I couldn't get that to work.

  • Move test, remove triple

Probably just IPO then. It's still not WebAssembly specific.

Please update the patch description.

Is there a good place to put the testcase? I guess test/Other?

Better would be the appropriate subdirectory of test/Transforms. Although you should split your test - one for dead arg elim and one for arg promotion.

What happens without this patch - incorrect behavior or other missed optimization opportunities? Are we losing anything important by preventing the optimizations?

jgravelle-google retitled this revision from Don't modify LibFuncs in LTO to Don't modify LibFuncs in DeadArgumentElimination or ArgumentPromotion.Jun 21 2018, 12:22 PM
jgravelle-google edited the summary of this revision. (Show Details)

Better would be the appropriate subdirectory of test/Transforms.

In theory because the actual crash is in DeadStoreElimination I could put it there?

Although you should split your test - one for dead arg elim and one for arg promotion.

I figured because it was the same input bitcode that causes issues it made sense to keep it as one file. Although it might make more sense to have it be two separate calls to opt to make it clearer.
The other thought was I don't want *any* passes in -std-link-opts to cause this issue, so this test should catch new passes that overlook the same weird edge case.
The minimal subset of reproducing passes for the first case is -internalize -ipsccp -deadargelim -dse

What happens without this patch - incorrect behavior or other missed optimization opportunities? Are we losing anything important by preventing the optimizations?

Edited the commit message to make it clearer what the problem was - we crash in DeadStoreElimination. (Specifically here: https://github.com/llvm-mirror/llvm/blob/5119d140c565456349a27c45b52be5a81b18ffe0/lib/Transforms/Scalar/DeadStoreElimination.cpp#L216)

lebedev.ri added inline comments.
lib/Transforms/IPO/ArgumentPromotion.cpp
831–832

This reads weird.
Are you sure you didn't mean:

// Don't promote arguments for library functions,
// because later passes may make assumptions about library function signatures.

Maybe instead of trying to turn off optimizations on internal builtin functions, we should instead try to make optimizations not treat functions with local linkage as builtin? Not sure. This came up before on llvmdev, but we didn't really decide one way or the other.

If you're going to go with this approach, we probably also need to fix a bunch of other places that call hasLocalLinkage: llvm::canTrackArgumentsInterprocedurally is used by IPSCCP, GlobalOpt will change the calling convention of function definitions, etc.

Regardless of which approach we choose, DSE should be fixed to use getLibFunc() so it doesn't crash on invalid input.

Also, LTO currently calls TargetLibraryInfo::disableAllFunctions to avoid this problem.

Just ran across this again. @jgravelle-google is this still relevant?

lib/Transforms/IPO/ArgumentPromotion.cpp
831–832

+1, this is better wording (and is the intent for this change).