Fix numpy reader crash #4466
Fix numpy reader crash #4466
Conversation
| @@ -16,6 +16,7 @@ | |||
| #define DALI_IMGCODEC_DECODERS_MEMORY_POOL_H_ | |||
|
|
|||
| #include <atomic> | |||
| #include <array> | |||
There was a problem hiding this comment.
This failed to compile with libc++ 12
… test. Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
mzient
force-pushed
the
FixNumpyReaderCrash
branch
from
637f784
to
2ffca2d
Compare
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
|
CI MESSAGE: [6591353]: BUILD STARTED |
| @@ -43,6 +44,12 @@ NumpyReaderGPU::NumpyReaderGPU(const OpSpec& spec) | |||
| kmgr_transpose_.Resize<TransposeKernel>(1); | |||
| } | |||
|
|
|||
| NumpyReaderGPU::~NumpyReaderGPU() { | |||
There was a problem hiding this comment.
Moved as-is from the header.
|
|
||
| void Close() { | ||
| if ((fd != -1) && (fdd != -1)) | ||
| if (cufh) { |
There was a problem hiding this comment.
Use the real value we're interested in instead of relying on side effects.
| @@ -147,6 +147,11 @@ MmapedFileStream::MmapedFileStream(const std::string& path, bool read_ahead) : | |||
| DALI_ENFORCE(p_ != nullptr, "Could not open file " + path + ": " + std::strerror(errno)); | |||
| } | |||
|
|
|||
| MmapedFileStream::~MmapedFileStream() { | |||
There was a problem hiding this comment.
Moved from the header.
| // set the path to current path | ||
| path_ = path; | ||
| } | ||
|
|
||
| StdCUFileStream::~StdCUFileStream() { |
There was a problem hiding this comment.
Moved from the header.
| @@ -29,6 +29,11 @@ StdFileStream::StdFileStream(const std::string& path) : FileStream(path) { | |||
| DALI_ENFORCE(fp_ != nullptr, "Could not open file " + path + ": " + std::strerror(errno)); | |||
| } | |||
|
|
|||
| StdFileStream::~StdFileStream() { | |||
There was a problem hiding this comment.
Moved from the header.
|
CI MESSAGE: [6591353]: BUILD FAILED |
| with open(cursor.location.file.name, 'r', encoding='latin-1') as file: | ||
| start = cursor.extent.start.offset | ||
| end = cursor.extent.end.offset | ||
| declaration = file.read()[start:end] |
There was a problem hiding this comment.
This copied the declaration from the original file - it didn't work correctly, because there might have been some macros - and they could be used in different order, yielding different results:
#define cuFileDriverClose cuFileDriverClose_v2
CUFileError_t cuFileDriverClose();or
CUFileError_t cuFileDriverClose_v2();
#define cuFileDriverClose cuFileDriverClose_v2These would produce the same preprocessed output, but the declaration would be different.
| def function_header(return_type, name, args): | ||
| arg_str = ", ".join([f"{arg_type} {arg_name}" for arg_type, arg_name in args]) | ||
| ret = f"{return_type} {name}({arg_str})" | ||
| return ret |
There was a problem hiding this comment.
Recreate the function header with the actual function name (punching through macros).
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
|
CI MESSAGE: [6591414]: BUILD STARTED |
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
|
CI MESSAGE: [6591873]: BUILD STARTED |
|
CI MESSAGE: [6591414]: BUILD FAILED |
|
CI MESSAGE: [6591873]: BUILD PASSED |
| std::swap(fd, other.fd); | ||
| std::swap(fdd, other.fdd); | ||
| std::swap(cufh, other.cufh); | ||
| other.Close(); |
There was a problem hiding this comment.
Do you have to explicitly call Close? Wouldn't it just be called when destructing other?
There was a problem hiding this comment.
I read somewhere that implementing move assignment as a pure swap isn't a good practice, because it prolongs the life of the object being overwritten in a surprising way. This is especially important in case of resources like files, which have global side effects. Consider:
File a = File::open("a.dat", "rw");
File b = File::open("b.dat", "rw");
a = std::move(b); // the user would expect that "a.dat" is closed, but this assignment actually did a swap
File c = File::open("a.dat", "rw"); // file busy, because the moved-out object `b` is keeping the file open.There was a problem hiding this comment.
Huh. That's interesting. I didn't know how the move works, then. I thought b would be destroyed just after evaluation of this assignment.
There was a problem hiding this comment.
We learn something new every day ;) The object still "lives", even though it shouldn't be used for anything other than destruction. I'm not sure if the compiler is allowed to destroy a moved-out local variable before the scope end, but there are certainly cases where you can move-out objects with more permanent storage - so they cannot be destroyed (calling a destructor multiple times is an error). For example, you can loop over an array and move every element from it to some other container and then destroy the array. The destructors will be called when the array is destroyed, not when each object in the array is moved out.
Category:
Bug fix (non-breaking change which fixes an issue)
Description:
The new libcufile doesn't exit cleanly (for some reasons) and has problems with library use count tracking.
This PR does the following:
Additional information:
Affected modules and functionalities:
NumpyReaderGPU
dynlink stubs
Key points relevant for the review:
Tests:
GDSMemTest (GTest)
NumpyReader tests (reader/test_numpy.py)
Checklist
Documentation
DALI team only
Requirements
REQ IDs: N/A
JIRA TASK: N/A