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.

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

libomptarget/src/omptarget.cpp:440: bad erase ? #38052

Closed
llvmbot opened this issue Aug 25, 2018 · 6 comments
Closed

libomptarget/src/omptarget.cpp:440: bad erase ? #38052

llvmbot opened this issue Aug 25, 2018 · 6 comments
Labels
bugzilla Issues migrated from bugzilla openmp

Comments

@llvmbot
Copy link
Collaborator

llvmbot commented Aug 25, 2018

Bugzilla Link 38704
Resolution FIXED
Resolved on Sep 04, 2018 08:16
Version unspecified
OS Linux
Reporter LLVM Bugzilla Contributor
CC @hahnjo,@hfinkel
Fixed by commit(s) r341371

Extended Description

openmp-7.0.0rc1.src/libomptarget/src/omptarget.cpp:419] ->
openmp-7.0.0rc1.src/libomptarget/src/omptarget.cpp:440]: (error) Iterator 'it' used after element has been erased.

Source code is

Device.ShadowPtrMap.erase(it);

maybe better code

it = Device.ShadowPtrMap.erase(it);
@hahnjo
Copy link
Member

hahnjo commented Aug 26, 2018

I think you are right. https://en.cppreference.com/w/cpp/container/map/erase: "References and iterators to the erased elements are invalidated." I guess you are not allowed to call operator++() on invalid iterators?

Can you please give some more context: Is that output from a sanitizer? If yes, which one? What did the "input" program look like, ie what are you mapping?

@llvmbot
Copy link
Collaborator Author

llvmbot commented Aug 26, 2018

I think you are right.
https://en.cppreference.com/w/cpp/container/map/erase: "References and
iterators to the erased elements are invalidated." I guess you are not
allowed to call operator++() on invalid iterators?

Agreed. There isn't much you can do with an invalid iterator,
since it is invalid.

The example code in your link shows how to do it right.

Can you please give some more context: Is that output from a sanitizer? If
yes, which one?

I am not sure what you mean by sanitiser, but I did run an analyser
called cppcheck over the code.

What did the "input" program look like, ie what are you mapping?

I don't understand this question.

@hahnjo
Copy link
Member

hahnjo commented Aug 26, 2018

Can you please give some more context: Is that output from a sanitizer? If
yes, which one?

I am not sure what you mean by sanitiser, but I did run an analyser
called cppcheck over the code.

What did the "input" program look like, ie what are you mapping?

I don't understand this question.

Ah ok, so this is purely from static analysis? In particular you are not running any OpenMP application that uses target regions?

@llvmbot
Copy link
Collaborator Author

llvmbot commented Aug 26, 2018

Ah ok, so this is purely from static analysis?

Yes.

In particular you are not running any OpenMP application that uses target regions?

Correct.

@hahnjo
Copy link
Member

hahnjo commented Sep 4, 2018

I posted https://reviews.llvm.org/D51623 for review. I was able to craft a test case that was crashing without this patch, thanks for reporting!

@hahnjo
Copy link
Member

hahnjo commented Sep 4, 2018

Landed in r341371.

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 10, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla openmp
Projects
None yet
Development

No branches or pull requests

2 participants