Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

False negative with stack slot reuse #590

Open
ramosian-glider opened this issue Sep 1, 2015 · 6 comments
Open

False negative with stack slot reuse #590

ramosian-glider opened this issue Sep 1, 2015 · 6 comments
Assignees
@ramosian-glider
Labels
Priority-Medium ProjectMemorySanitizer Status-New Type-Defect

Comments

@ramosian-glider
Copy link
Member

Originally reported on Google Code with ID 87

int main() {
  for (int i = 0; i < 10000; ++i) {
    int a;
    int * volatile p = &a;
    if (i < 9000)
      *p = i;
    else
      return *p;
  }
  return 0;
}

Local variable "a" should be poisoned every time it goes into scope (on llvm.lifetime.start?).

Reported by eugenis@chromium.org on 2015-03-02 11:46:01
@ramosian-glider
Copy link
Member Author 

An unfinished patchset.
It fixes this issue, but runs into another - lifetime intrinsics break debug location
for the return instruction in some cases.

Reported by eugenis@google.com on 2015-04-06 20:18:35

- _Attachment: [lifetime.patch](https://storage.googleapis.com/google-code-attachments/memory-sanitizer/issue-87/comment-1/lifetime.patch)_ - _Attachment: [lifetime-cfe.patch](https://storage.googleapis.com/google-code-attachments/memory-sanitizer/issue-87/comment-1/lifetime-cfe.patch)_

@kcc kcc assigned eugenis and unassigned google Dec 2, 2015
@kcc
Copy link
Contributor

kcc commented Feb 28, 2017

@davidben FYI

@morehouse
Copy link
Contributor

@eugenis: Any progress on this?

@eugenis
Copy link
Contributor

eugenis commented Jun 8, 2018

No.

@ramosian-glider
Copy link
Member Author

Re-uploaded the patch for posterity.
We're hitting the same problem in KMSAN.
lifetime.txt

@ramosian-glider ramosian-glider mentioned this issue Apr 5, 2019
Closed
llvm-git-migration pushed a commit to llvm/llvm-project that referenced this issue Apr 30, 2019
@ramosian-glider
MSan: handle llvm.lifetime.start intrinsic
06d00af 
Summary:
When a variable goes into scope several times within a single function
or when two variables from different scopes share a stack slot it may
be incorrect to poison such scoped locals at the beginning of the
function.
In the former case it may lead to false negatives (see
google/sanitizers#590), in the latter - to
incorrect reports (because only one origin remains on the stack).

If Clang emits lifetime intrinsics for such scoped variables we insert
code poisoning them after each call to llvm.lifetime.start().
If for a certain intrinsic we fail to find a corresponding alloca, we
fall back to poisoning allocas for the whole function, as it's now
impossible to tell which alloca was missed.

The new instrumentation may slow down hot loops containing local
variables with lifetime intrinsics, so we allow disabling it with
-mllvm -msan-handle-lifetime-intrinsics=false.

Reviewers: eugenis, pcc

Subscribers: hiraditya, llvm-commits

Tags: #llvm

Differential Revision: https://reviews.llvm.org/D60617

llvm-svn: 359536
dtzWill pushed a commit to llvm-mirror/llvm that referenced this issue Apr 30, 2019
@ramosian-glider
MSan: handle llvm.lifetime.start intrinsic
a5ffeed 
Summary:
When a variable goes into scope several times within a single function
or when two variables from different scopes share a stack slot it may
be incorrect to poison such scoped locals at the beginning of the
function.
In the former case it may lead to false negatives (see
google/sanitizers#590), in the latter - to
incorrect reports (because only one origin remains on the stack).

If Clang emits lifetime intrinsics for such scoped variables we insert
code poisoning them after each call to llvm.lifetime.start().
If for a certain intrinsic we fail to find a corresponding alloca, we
fall back to poisoning allocas for the whole function, as it's now
impossible to tell which alloca was missed.

The new instrumentation may slow down hot loops containing local
variables with lifetime intrinsics, so we allow disabling it with
-mllvm -msan-handle-lifetime-intrinsics=false.

Reviewers: eugenis, pcc

Subscribers: hiraditya, llvm-commits

Tags: #llvm

Differential Revision: https://reviews.llvm.org/D60617

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@359536 91177308-0d34-0410-b5e6-96231b3b80d8
@ramosian-glider
Copy link
Member Author

This is fixed in https://reviews.llvm.org/D60617, but we also need to remove a check in shouldEmitLifetimeMarkers().

earl pushed a commit to earl/llvm-mirror that referenced this issue Apr 30, 2019
@ramosian-glider
MSan: handle llvm.lifetime.start intrinsic
7bff1d0 
Summary:
When a variable goes into scope several times within a single function
or when two variables from different scopes share a stack slot it may
be incorrect to poison such scoped locals at the beginning of the
function.
In the former case it may lead to false negatives (see
google/sanitizers#590), in the latter - to
incorrect reports (because only one origin remains on the stack).

If Clang emits lifetime intrinsics for such scoped variables we insert
code poisoning them after each call to llvm.lifetime.start().
If for a certain intrinsic we fail to find a corresponding alloca, we
fall back to poisoning allocas for the whole function, as it's now
impossible to tell which alloca was missed.

The new instrumentation may slow down hot loops containing local
variables with lifetime intrinsics, so we allow disabling it with
-mllvm -msan-handle-lifetime-intrinsics=false.

Reviewers: eugenis, pcc

Subscribers: hiraditya, llvm-commits

Tags: #llvm

Differential Revision: https://reviews.llvm.org/D60617

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@359536 91177308-0d34-0410-b5e6-96231b3b80d8
dylanmckay pushed a commit to dylanmckay/llvm that referenced this issue May 16, 2019
MSan: handle llvm.lifetime.start intrinsic
c6c0893 
Summary:
When a variable goes into scope several times within a single function
or when two variables from different scopes share a stack slot it may
be incorrect to poison such scoped locals at the beginning of the
function.
In the former case it may lead to false negatives (see
google/sanitizers#590), in the latter - to
incorrect reports (because only one origin remains on the stack).

If Clang emits lifetime intrinsics for such scoped variables we insert
code poisoning them after each call to llvm.lifetime.start().
If for a certain intrinsic we fail to find a corresponding alloca, we
fall back to poisoning allocas for the whole function, as it's now
impossible to tell which alloca was missed.

The new instrumentation may slow down hot loops containing local
variables with lifetime intrinsics, so we allow disabling it with
-mllvm -msan-handle-lifetime-intrinsics=false.

Reviewers: eugenis, pcc

Subscribers: hiraditya, llvm-commits

Tags: #llvm

Differential Revision: https://reviews.llvm.org/D60617

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@359536 91177308-0d34-0410-b5e6-96231b3b80d8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ramosian-glider ramosian-glider

Labels
Priority-Medium ProjectMemorySanitizer Status-New Type-Defect
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@eugenis @ramosian-glider @kcc @morehouse