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

fixed bad alloc and added missing unix lv_interop function loading #372

Merged
merged 4 commits into from
Dec 4, 2024

Conversation

j-medland
Copy link
Contributor

I am working on some changes to the C++ build tooling and dll linkage (see #144) and noticed that the memory size allocation in OccurServerEvent function is likely 1 byte too big on 32-bit platforms and 3 bytes too small on 64-bit platforms (dependant on the LV array/string byte-packing) . You would have to be pretty unlucky to have something important allocated in the bytes that get "borrowed" on 64-bit systems but it seemed worth fixing.

As we are trying to create a LabVIEW-string from a std::string we can use the existing SetLVString function to allocate and copy the std::string into something we can use to call PostUserEvent.

Clean-up of the handle is done with DSDisposeHandle

Strangley, InitCallbacks function only gets the address of DSDisposeHandle on Windows so I added the code so both platforms load the same functions.

Finally - I removed the call in InitCallbacks which would make the LabVIEW IDE or the RTE exit on a shared-library load error on Linux platforms. It is better to rely on LabVIEW's rudimentary shared-library call fault handling than to just make the whole IDE/Application exit with little indication to the user as to what happened but I am happy to discuss if you feel differently

Cheers

@jasonmreding
Copy link
Collaborator

This clearly seems like an improvement to me. However, I can't speak to the exit behavior under Linux. This behavior has already changed once as seen from this PR. Given the exit behavior already seemed questionable, I don't imagine there will be too much push back, but hopefully Cifra can comment. I don't imagine these bindings ever really fail in practice.

@j-medland j-medland force-pushed the fix-bad-malloc-occur-server-event branch from 3663d2e to 82217d6 Compare September 23, 2024 21:01
@j-medland
Copy link
Contributor Author

Sorry that the tests need to be rerun. I force pushed a commit with --signoff as per the contribution guide update

@j-medland
Copy link
Contributor Author

Hello - it would be great to get this reviewed or some more information if there is something I can do to help this happen?

I appreciate NI making this open source and I understand if your aren't in a position to accept community PRs but it would be great to know. I would also be happy if this was just fixed by one of the maintainers.

@nischalks nischalks requested review from yash-ni and removed request for ccifra October 10, 2024 06:39
@jasonmreding jasonmreding merged commit 910072f into ni:master Dec 4, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants