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

Prepatory work for supporting -Wthread-safety #1461

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

seanm
Copy link
Contributor

@seanm seanm commented Feb 5, 2024

No description provided.

@seanm
Copy link
Contributor Author

seanm commented Feb 5, 2024

This is prep work for #1419.

It is ready for reviewing and merging.

@mcuee mcuee added the core Related to common codes label Feb 5, 2024
@seanm
Copy link
Contributor Author

seanm commented Feb 5, 2024

I should add: -Wthread-safety only works fully in C++. Luckily, libusb can be made to (optionally) build as C++ with these modifications.

I'm not proposing to change the language libusb builds as by default. I'm proposing it be both valid C and valid C++ so that we can use -Wthread-safety fully.

@@ -2467,14 +2467,14 @@ int API_EXPORTED libusb_init_context(libusb_context **ctx, const struct libusb_i
list_init(&_ctx->open_devs);

/* apply default options to all new contexts */
for (enum libusb_option option = 0 ; option < LIBUSB_OPTION_MAX ; option++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this? Enum not allowed as counter?

Copy link
Contributor Author

@seanm seanm Feb 5, 2024

Choose a reason for hiding this comment

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

The old way, two errors:

core.c:2470:26 Cannot initialize a variable of type 'enum libusb_option' with an rvalue of type 'int';
core.c:2470:74 Cannot increment expression of enum type 'enum libusb_option'

libusb/descriptor.c Outdated Show resolved Hide resolved
libusb/os/darwin_usb.c Outdated Show resolved Hide resolved
@@ -1666,8 +1668,8 @@ static const struct libusb_interface_descriptor *get_interface_descriptor_by_num
return NULL;
}

static enum libusb_error get_endpoints (struct libusb_device_handle *dev_handle, uint8_t iface) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are some function changed to return int instead of enum?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To pacify the C++ compiler, which is more type strict and does not allow such freewheeling conversions between enums and integers.

Copy link
Member

@hjelmn hjelmn Feb 12, 2024

Choose a reason for hiding this comment

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

Weird. Does it have to use the C++ compiler. There will be issues if it does because C++ is not a superset of C and some C11 features do not work in C++20.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it have to use the C++ compiler.

-Wthread-safety partly works in C. The big issue is: llvm/llvm-project#20777

C++ is not a superset of C

Indeed, but it mostly is. At least enough for current libusb master.

Again, I'm not wanting to actually switch libusb's build system to C++. I'm only saying it's handy to do so occasionally, to turn -Wthread-safety on occasionally, like as check before cutting a new release.

Copy link
Member

Choose a reason for hiding this comment

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

Hah, they have been trying to figure out what to do for 10 years?! I see some reasonable options but little movement. There is no technical reason why thread analysis should not work with C.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hah, they have been trying to figure out what to do for 10 years?!

Exactly, alas. So to get the full abilities of -Wthread-safety, we need to build libusb as C++ (or start patching clang). Hence this PR. :)

@seanm
Copy link
Contributor Author

seanm commented May 5, 2024

@tormodvolden would be nice to get this merged... I really think libusb could benefit from -Wthread-safety, it has found bugs for us already...

@tormodvolden
Copy link
Contributor

Is this replacing #1419 ?

@seanm
Copy link
Contributor Author

seanm commented May 8, 2024

Is this replacing #1419 ?

No. This is prep for for #1419. #1419 will add the actual annotations themselves.

@seanm seanm force-pushed the clang-thread-safety-prep branch from 7e6a70e to 9ae7460 Compare May 27, 2024 18:49
@seanm seanm force-pushed the clang-thread-safety-prep branch from 9ae7460 to b65d8eb Compare May 29, 2024 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Related to common codes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants