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

[Windows] Use portable version of strerror #2870

Merged
merged 1 commit into from
Jan 22, 2025

Conversation

gkisalapl
Copy link
Contributor

This PR is part of porting nntrainer to windows.
This PR replace using GNU specific function strerror_r with portable version std::strerror

Replace this:
https://linux.die.net/man/3/strerror_r
With:
https://en.cppreference.com/w/cpp/string/byte/strerror

Self-evaluation:

Build test: [X]Passed [ ]Failed [ ]Skipped
Run test: [X]Passed [ ]Failed [ ]Skipped

Copy link
Contributor

@EunjuYang EunjuYang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Please check the CI.

Copy link
Member

@skykongkong8 skykongkong8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have to apply clang-format in order to pass the CI.
Please check the clang-format complaint, unittest_tflite_export.cpp

@gkisalapl
Copy link
Contributor Author

You have to apply clang-format in order to pass the CI. Please check the clang-format complaint, unittest_tflite_export.cpp

I've run clang format, using following version:

$ clang-format --version
$ Ubuntu clang-format version 14.0.0-1ubuntu1.1

but it seems that on continues integration we're using some different version.
Do you know which exactly ?

@djeong20
Copy link
Contributor

djeong20 commented Jan 15, 2025

Hi @gkisalapl,

I wanted to let you know that I applied your work to my local setup and confirmed that my results match yours in unittest_tflite_export.cpp. I’m currently using clang-format version 10 on Ubuntu 20.04. Our CI environment operates on Ubuntu 24.04, which seems to be using a newer version of clang-format, likely version 16 or 18. For now, I think we can disregard this issue.

Copy link
Contributor

@baek2sm baek2sm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The strerror_r() function is similar to strerror(), but strerror_r() is thread safe function.
In Windows, a thread-safe function named strerror_s() is provided separately.
Nevertheless, will there be no issue when this modification is applied?

@gkisalapl
Copy link
Contributor Author

gkisalapl commented Jan 16, 2025

The strerror_r() function is similar to strerror(), but strerror_r() is thread safe function. In Windows, a thread-safe function named strerror_s() is provided separately. Nevertheless, will there be no issue when this modification is applied?

Good point, thank you for this finding. In order to be 100% sure that we keep current functionality we should do:

#ifdef _WIN32
strerror_s(...)
#else
stderror_r(...)
#endif

but If we stick with portable non thread safe version stderror() the only drawback is that subsequent calls to strerror() may overwrite the buffer addressed by the returned pointer and print incorrect error message in result.
In order to make it happen we would need to report error by strerror() in 2 threads in similar time which is very unique situation I think. My recommendation is to go with portable version instead of making os specific calls.

Copy link
Contributor

@djeong20 djeong20 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! please rebase to pass the CI 💯

Replace using GNU specific function strerror_r with portable version std::strerror

**Self-evaluation:**
1. Build test: [X]Passed [ ]Failed [ ]Skipped
2. Run test:   [X]Passed [ ]Failed [ ]Skipped

Signed-off-by: Grzegorz Kisala <[email protected]>
@myungjoo myungjoo merged commit 719cc1d into nnstreamer:main Jan 22, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR/OK2GO CI failed but okay to merge/review PR/READY2MERGE
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants