Skip to content

mutable noalias: re-enable permanently, only for panic=abort, or stabilize flag? #45029

Closed
@Gankra

Description

@Gankra
Contributor

In #29485 it was discovered that llvm was miscompiling noalias in the context of unwind edges. Further sleuthing determined that it was only a problem with mutable references, and the performance impact was fairly small.

In #31545 we removed noalias from mutable references based on this information.

In #45012 I added a -Zmutable-noalias flag to opt back into the old behaviour.

I am told many of the llvm bugs have been fixed (and may be completely fixed by some in-progress rewrites of the relevant components?). Also, we now have a -Cpanic=abort to disable unwinding altogether, which should be immune to the original problem.

This leaves us with three options moving forward (not necessarily mutually exclusive):

  • Promote -Zmutable-noalias to a stable -C option, so stable users can opt in
  • Enable this internally whenever -Cpanic=abort is set
  • Enable this internally always (unclear if sound today, or when it would be)

I have no particularly strong preference here; all of these options serve my purpose perfectly fine (make stable codegen better for gecko).

Activity

Gankra

Gankra commented on Oct 4, 2017

@Gankra
ContributorAuthor
sunfishcode

sunfishcode commented on Oct 4, 2017

@sunfishcode
Member

@eefriedman's non-comprensive audit in #29485 also turned up -memcpyopt, -mldst-motion, and -loop-idiom. At a brief glance, -loop-idiom looks fixed (r274673). It doesn't appear that -memcpyopt or -mldst-motion are fixed yet (-mldst-motion wasn't enabled by default earlier, but it is now). And for safety's sake, it would be good to have a more comprehensive audit.

Gankra

Gankra commented on Oct 4, 2017

@Gankra
ContributorAuthor

@sunfishcode is it accurate to say that the only concern is the interaction with unwinding, in which case enabling for panic=abort should be fine?

pcwalton

pcwalton commented on Oct 4, 2017

@pcwalton
Contributor

"Enable this internally whenever -Cpanic=abort is set" seems like a completely uncontroversial thing we can do now.

sunfishcode

sunfishcode commented on Oct 4, 2017

@sunfishcode
Member

Yes, the interaction with unwinding is the only concern I'm aware of here. Are panics the only unwinding possible under panic=abort, or does Rust support, for example, calling C++ code which throws?

hanna-kruppe

hanna-kruppe commented on Oct 4, 2017

@hanna-kruppe
Contributor

Unwinding across language barriers is UB.

sunfishcode

sunfishcode commented on Oct 4, 2017

@sunfishcode
Member

Various other language implementations related to Rust's do support cross-language unwinding. Is it specifically UB in Rust?

hanna-kruppe

hanna-kruppe commented on Oct 4, 2017

@hanna-kruppe
Contributor

At least Rust unwinding across an FFI barrier has definitely been declared UB (cf. #18510). I am relatively sure the reverse situation is also UB – or at least, I don't think it has ever been documented as working. Due to different personality routines, I'd expect all hell breaking loose if (say) C++ code throws an exception and it propagates to a Rust landing pad, and we're not in the habit of making such weird things defined depending on codegen options.

Gankra

Gankra commented on Oct 5, 2017

@Gankra
ContributorAuthor

I added the panic=abort automatic opt-in to #45012

added
T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.
on Oct 5, 2017
bstrie

bstrie commented on Oct 7, 2017

@bstrie
Contributor

I don't see why we would ever add a stable -C flag for this. If it doesn't miscompile code, then it should be on by default, no flag needed. If it does miscompile code, then it would be an enormous departure of policy for us to offer such a flag; refusing to add memory-unsafe compiler flags is part of the reason that we've refused to add a flag to completely disable automatic bounds checks. Am I wrong in that this would be completely unprecedented?

glaebhoerl

glaebhoerl commented on Oct 10, 2017

@glaebhoerl
Contributor

Given that no one has suggested otherwise, am I right to infer that this optimization is going to be valid under any conceivable formulation of the UB guidelines / memory model we might eventually settle on?

added
A-LLVMArea: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues.
C-enhancementCategory: An issue proposing an enhancement or a PR with one.
on Jan 22, 2018
RalfJung

RalfJung commented on May 14, 2018

@RalfJung
Member

This is kind-of a duplicate of #31681, is it not?


am I right to infer that this optimization is going to be valid under any conceivable formulation of the UB guidelines / memory model we might eventually settle on?

Modulo the fact that the exact scope of optimizations enabled by adding noalias is not clear, this is definitely a design goal. Concretely, the optimization at https://godbolt.org/g/64CjDX is definitely intended to be allowed:

pub fn foo(x: &mut i32, y: &mut i32) -> i32 {
    *x = 23;
    *y = 19;
    *x // optimize to 23
}
nikic

nikic commented on Nov 3, 2018

@nikic
Contributor

I believe this issue can be closed in favor of #54878, which tracks the LLVM bug currently blocking this. Prior to that issue noalias metadata was always emitted (with opt-out flag), and once the issue is resolved I'll expect we'll go back to emitting it.

Phlosioneer

Phlosioneer commented on Jul 31, 2019

@Phlosioneer
Contributor

Triage: Can this issue be closed in favor of #54878 ? nikic commented above but received no response.

Mark-Simulacrum

Mark-Simulacrum commented on Jul 31, 2019

@Mark-Simulacrum
Member

Yes, that seems reasonable. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    A-LLVMArea: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues.C-enhancementCategory: An issue proposing an enhancement or a PR with one.T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @pcwalton@nikic@RalfJung@nagisa@bstrie

        Issue actions

          mutable noalias: re-enable permanently, only for panic=abort, or stabilize flag? · Issue #45029 · rust-lang/rust