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.

Sign up for GitHub

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

Jump to bottom

Unify code that uses GetProcAddress on Windows #12581

Closed
wants to merge 1 commit into from
Closed

Unify code that uses GetProcAddress on Windows #12581

wants to merge 1 commit into from

Conversation

pps83
Copy link
Contributor

@pps83 pps83 commented Dec 21, 2023

  • avoid using CURLX_FUNCTION_CAST, instead dereference destination pointer to FARPROC*
  • remove unnecessary function pointer typedefs required for CURLX_FUNCTION_CAST

@pps83
Copy link
Contributor Author

pps83 commented Dec 21, 2023

Note, there were things like APIENTRY vs WINAPI (in case of InitSecurityInterface, LoadLibraryEx, or RtlVerifyVersionInfo). APIENTRY is always defined to WINAPI in the headers.

@jay
Copy link
Member

jay commented Dec 21, 2023

I think there is some reason we are using CURLX_FUNCTION_CAST, probably to avoid some compiler's warnings.

@jay jay added build Windows Windows-specific labels Dec 21, 2023
@pps83
Copy link
Contributor Author

pps83 commented Dec 21, 2023 •
edited

I think there is some reason we are using CURLX_FUNCTION_CAST, probably to avoid some compiler's warnings.

Yes, to cast from FARPROC (return value of GetProcAddress) you need to cast to some function pointer type. This makes it obscure: you have to typedef function pointer type etc. The other way around: if you cast destination function pointer to FARPROC (which is int (WINAPI *)(void)) then you directly assign return value of GetProcAddress without messing with CURLX_FUNCTION_CAST.

@jay
Copy link
Member

jay commented Dec 21, 2023

@MarcelRaad 25d2a1b

@pps83
Unify code that uses GetProcAddress on Windows
8154940 
 + avoid using CURLX_FUNCTION_CAST, instead dereference destination pointer to FARPROC*
 + remove unnecessary function pointer typedefs required for CURLX_FUNCTION_CAST
@pps83
Copy link
Contributor Author

pps83 commented Dec 22, 2023 •
edited

@bagder looks like there is an assert in a test (unrelated to the change): https://github.com/curl/curl/actions/runs/7300608761/job/19895602267?pr=12581#step:21:538

FAILED tests/http/test_07_upload.py::TestUpload::test_07_22_upload_parallel_fail[0-h3] - AssertionError: status #0 exitcode: expected 95, got 55

@MarcelRaad
Copy link
Member

Unfortunately, this results in strict-aliasing warnings-as-errors on MinGW-w64 with GCC 10 when targeting x64, and so does the recently merged code in a6bbc87#diff-1745e851717837ff181d2de193e6c8b077ec5ea3159a2366740cb66b9c033325. The warning shows up in the autobuilds too:
https://curl.se/dev/log.cgi?id=20231226050249-808232#prob1

@jay
Copy link
Member

jay commented Dec 27, 2023

Unfortunately, this results in strict-aliasing warnings-as-errors on MinGW-w64 with GCC 10 when targeting x64, and so does the recently merged code

What do you recommend? Reject this and change the existing instances to CURLX_FUNCTION_CAST?

@MarcelRaad
Copy link
Member

Yes, unless there's another way that doesn't result in new warnings, I'd consistently use CURLX_FUNCTION_CAST instead. Unfortunate that the warning doesn't directly show up in the GitHub CI.

jay added a commit to jay/curl that referenced this pull request Dec 28, 2023
@jay
system_win32: fix a function pointer assignment warning
ef4fb8d 
- Use CURLX_FUNCTION_CAST to suppress a function pointer assignment
  warning.

a6bbc87 added lookups of some Windows API functions and then cast them
like *(FARPROC*)&Curl_funcname = address. Some versions of gcc warn
about that as breaking strict-aliasing rules so this PR changes those
assignments to use CURLX_FUNCTION_CAST.

Bug: curl#12581 (comment)
Reported-by: Marcel Raad

Closes #xxxx
jay added a commit to jay/curl that referenced this pull request Dec 28, 2023
@jay
system_win32: fix a function pointer assignment warning
a9ebe3e 
- Use CURLX_FUNCTION_CAST to suppress a function pointer assignment
  warning.

a6bbc87 added lookups of some Windows API functions and then cast them
like *(FARPROC*)&Curl_funcname = address. Some versions of gcc warn
about that as breaking strict-aliasing rules so this PR changes those
assignments to use CURLX_FUNCTION_CAST.

Bug: curl#12581 (comment)
Reported-by: Marcel Raad

Closes #xxxx
@jay jay mentioned this pull request Dec 28, 2023
Closed
@jay
Copy link
Member

jay commented Dec 28, 2023 •
edited

Closing in favor of #12581 #12602.

@jay jay closed this Dec 28, 2023
jay added a commit that referenced this pull request Dec 28, 2023
@jay
system_win32: fix a function pointer assignment warning
26f002e 
- Use CURLX_FUNCTION_CAST to suppress a function pointer assignment
  warning.

a6bbc87 added lookups of some Windows API functions and then cast them
like `*(FARPROC*)&Curl_funcname = address`. Some versions of gcc warn
about that as breaking strict-aliasing rules so this PR changes those
assignments to use CURLX_FUNCTION_CAST.

Bug: #12581 (comment)
Reported-by: Marcel Raad

Closes #12602
@pps83
Copy link
Contributor Author

pps83 commented Dec 29, 2023

Yes, unless there's another way that doesn't result in new warnings, I'd consistently use CURLX_FUNCTION_CAST instead. Unfortunate that the warning doesn't directly show up in the GitHub CI.

Strange, those warnings should have resulted in warnings as error in CI. IMO, CI should be patched to ensure this happens.

@pps83 pps83 deleted the master-getprocaddr branch December 29, 2023 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
build Windows Windows-specific
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@pps83 @jay @MarcelRaad