This is an archive of the discontinued LLVM Phabricator instance.

[MemCpyOpt] Be conservative in the face of returns_twice calls
ClosedPublic

Authored by majnemer on May 23 2016, 9:17 PM.

Details

Summary

It is not safe to forward loads and stores around if calls to setjmp are
present: there might be a call which longjmps causing a memory location
to be initialized with an unexpected value.

This fixes PR27848.

Diff Detail

Event Timeline

majnemer updated this revision to Diff 58191.May 23 2016, 9:17 PM
majnemer retitled this revision from to [MemCpyOpt] Be conservative in the face of returns_twice calls.
majnemer updated this object.
majnemer added reviewers: rnk, eli.friedman, MatzeB, nlewycky.
majnemer added a subscriber: llvm-commits.
eli.friedman added inline comments.May 23 2016, 9:37 PM
lib/Transforms/Scalar/MemCpyOptimizer.cpp
1381

I don't think all the optimizations in this pass are affected; memset merging seems pretty safe. That said, probably not worth the trouble, given how rare functions which call setjmp are.

rnk edited edge metadata.May 24 2016, 11:57 AM

I think the right fix is declare that volatile loads and stores "capture" their pointer operands, or to tease out some other function attribute like that. Your reduced test case from IRC shows how we incorrectly redirect volatile stores from y to x: https://ghostbin.com/paste/v5n3f

majnemer updated this revision to Diff 58385.May 24 2016, 10:51 PM
majnemer edited edge metadata.

Move this change over to functionattrs.

majnemer updated this revision to Diff 58457.May 25 2016, 11:02 AM
  • Update the LangRef.
rnk accepted this revision.May 25 2016, 11:24 AM
rnk added a reviewer: sanjoy.
rnk edited edge metadata.

lgtm, but let's bug Sanjoy, the person who seems to think the hardest about IPO function attrs right now.

This revision is now accepted and ready to land.May 25 2016, 11:24 AM
sanjoy edited edge metadata.May 25 2016, 11:21 PM

I'm fine being bugged, but I think this review is missing some context -- how is "volatile stores escape the location being stored to" related to how we handle setjmp / longjmp?

A explicit test case on why we need this new rule will also be helpful (if not as a test case, then an example in the commit message), since @test_volatile only checks that we implement the rule correctly, but does not explain what the motivation is.

I'm fine being bugged, but I think this review is missing some context -- how is "volatile stores escape the location being stored to" related to how we handle setjmp / longjmp?

We substituted the underlying storage for a pointer when calling some function, that function would later perform a volatile store to that pointer. This is important because setjmp and longjmp require any modifications to objects which are loaded after the longjmp to be performed with volatile accesses. This all falls apart if the wrong underlying object is stored to.

A explicit test case on why we need this new rule will also be helpful (if not as a test case, then an example in the commit message), since @test_volatile only checks that we implement the rule correctly, but does not explain what the motivation is.

This revision was automatically updated to reflect the committed changes.