Page MenuHomePhabricator

[libomptarget] PR38704: Fix erase of ShadowPtrMap
ClosedPublic

Authored by Hahnfeld on Sep 4 2018, 6:04 AM.

Details

Summary

erase() invalidates the iterator and returns a new one pointing
to the following element. The code now follows the example at
https://en.cppreference.com/w/cpp/container/map/erase.
(The added testcase crashes without this patch.)

Diff Detail

Repository
rL LLVM

Event Timeline

Hahnfeld created this revision.Sep 4 2018, 6:04 AM
Hahnfeld edited the summary of this revision. (Show Details)Sep 4 2018, 6:05 AM
grokos accepted this revision.Sep 4 2018, 7:57 AM

Good catch, thanks for the fix!

This revision is now accepted and ready to land.Sep 4 2018, 7:57 AM
Hahnfeld added a comment.EditedSep 4 2018, 8:06 AM

Good catch, thanks for the fix!

Thank David Binderman and cppcheck ;-) (will have a link in the commit message)

This revision was automatically updated to reflect the committed changes.

make sense, thanks

Not related to this patch, but I think there is one more potentially incorrect use of containers in RTLsTy::RegisterLib(). This function may increase size of the global vector Devices when it initializes a new RTL that has not been yet used (see rtl.cpp line 219). This may result in memory reallocation and thus all pointers to the Devices elements that are stored in RTLs (see rtl.cpp line 228) would become invalid. I am not sure if these pointers are really used by RTLs now, but if yes, than it is a problem. RTLsTy::RegisterLib may be called more than once and each call may potentially initialize new RTLs.

Not related to this patch, but I think there is one more potentially incorrect use of containers in RTLsTy::RegisterLib(). This function may increase size of the global vector Devices when it initializes a new RTL that has not been yet used (see rtl.cpp line 219). This may result in memory reallocation and thus all pointers to the Devices elements that are stored in RTLs (see rtl.cpp line 228) would become invalid. I am not sure if these pointers are really used by RTLs now, but if yes, than it is a problem. RTLsTy::RegisterLib may be called more than once and each call may potentially initialize new RTLs.

Hehe, turns out: Yes and no!
Yes, this is undoubtly incorrect. But no, this can never happen because nobody is accessing the RTLInfoTy::Devices :D

Do you want to submit a patch removing Devices from RTLInfoTy?

Ok, I will submit such patch.