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

lib: fix unity builds with BearSSL, MSH3, Quiche, OmniOS #14932

Closed
wants to merge 4 commits into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Sep 16, 2024

  • fix MSH3 static symbol clash.
  • fix Quiche static symbol clash.
  • fix local macro clash with BearSSL header.
  • fix local macro clash with OmniOS system header.
    In file included from ../../lib/urldata.h:197,
                       from ../../lib/altsvc.c:32,
                       from libcurlall.c:2:
      ../../lib/cf-socket.h:55:25: error: expected ':', ',', ';', '}' or '__attribute__' before '.' token
         55 | #define sa_addr _sa_ex_u.addr
            |                         ^
      In file included from ../../lib/urldata.h:197,
                       from ../../lib/altsvc.c:32,
                       from libcurlall.c:2:
      ../../lib/cf-socket.h:55:25: error: expected ':', ',', ';', '}' or '__attribute__' before '.' token
         55 | #define sa_addr _sa_ex_u.addr
            |                         ^
    
    Ref: https://github.com/curl/curl/actions/runs/10738314933/job/29781644299?pr=14772#step:3:6115

Discovered while adding support for "unity" builds for autotools.

Required-by: #14922
Cherry-picked from #14815


w/o whitespace: https://github.com/curl/curl/pull/14932/files?w=1

@vszakats vszakats added the build label Sep 16, 2024
@vszakats
Copy link
Member Author

Proposing it for 8.10.1.

@bagder
Copy link
Member

bagder commented Sep 16, 2024

fix local macro clash with OmniOS system header.

sa_addr has been defined like this since 2011 and I'm pretty sure it has been built for OmniOS many times since. How come this becomes a problem in a unity build?

@vszakats
Copy link
Member Author

vszakats commented Sep 16, 2024

fix local macro clash with OmniOS system header.

sa_addr has been defined like this since 2011 and I'm pretty sure it has been built for OmniOS many times since. How come this becomes a problem in a unity build?

In "unity" all code ends up in a single source, making it catch more symbol clashes and mixups.

IOW it raises the risk for clashes when using non-namespaced local symbols because the namespace is filled with all the symbols used by libcurl, not just those pulled into an individual source file. Then, depending on the order of define/declare and use, it causes a redefinition issue, or messes up the user code that comes after the definition/declaration.

I think what happens here is that sa_addr gets defined in libcurl (via altsvc.c → urldata.h → cf-socket.h) before including an OmniOS header that happens to use this symbol, and breaks it. With non-unity this doesn't happen because the scope of the definition is more limited, and it's defined after including system headers.

sa_addr seems to be used in net/if.h in OmniOS:
https://github.com/omniosorg/illumos-omnios/blob/1a0b3ccac61c4543fa90d8e90de1a882e5491521/usr/src/uts/common/net/if.h#L415

edit: The issue would have surfaced earlier if the OmniOS job used cmake, but it happens to use autotools, so it only surfaced when unity mode arrived to autotools.

@dfandrich
Copy link
Contributor

Analysis of PR #14932 at f4037249:

Test 301 failed, which has NOT been flaky recently, so there could be a real issue in the PR.

Test 306 failed, which has NOT been flaky recently, so there could be a real issue in the PR.

Test 300 failed, which has NOT been flaky recently, so there could be a real issue in the PR.

Test 304 failed, which has NOT been flaky recently, so there could be a real issue in the PR.

Test 309 failed, which has NOT been flaky recently, so there could be a real issue in the PR.

Test 325 failed, which has NOT been flaky recently, so there could be a real issue in the PR.

Test 303 failed, which has NOT been flaky recently, so there could be a real issue in the PR.

There are more failures, but that's enough from Gha.

Generated by Testclutch

```
In file included from ../../lib/urldata.h:197,
                   from ../../lib/altsvc.c:32,
                   from libcurlall.c:2:
  ../../lib/cf-socket.h:55:25: error: expected ':', ',', ';', '}' or '__attribute__' before '.' token
     55 | #define sa_addr _sa_ex_u.addr
        |                         ^
  In file included from ../../lib/urldata.h:197,
                   from ../../lib/altsvc.c:32,
                   from libcurlall.c:2:
  ../../lib/cf-socket.h:55:25: error: expected ':', ',', ';', '}' or '__attribute__' before '.' token
     55 | #define sa_addr _sa_ex_u.addr
        |                         ^
```
Ref: https://github.com/curl/curl/actions/runs/10738314933/job/29781644299?pr=14772#step:3:6115
@vszakats vszakats closed this in aa1a153 Sep 19, 2024
@vszakats vszakats deleted the lib-unity-fixes branch September 19, 2024 18:27
moritzbuhl pushed a commit to moritzbuhl/curl that referenced this pull request Sep 20, 2024
- fix MSH3 static symbol clash.
- fix Quiche static symbol clash.
- fix local macro clash with BearSSL header.
- fix local macro clash with OmniOS system header.
  ```
  In file included from ../../lib/urldata.h:197,
                     from ../../lib/altsvc.c:32,
                     from libcurlall.c:2:
    ../../lib/cf-socket.h:55:25: error: expected ':', ',', ';', '}' or '__attribute__' before '.' token
       55 | #define sa_addr _sa_ex_u.addr
          |                         ^
    In file included from ../../lib/urldata.h:197,
                     from ../../lib/altsvc.c:32,
                     from libcurlall.c:2:
    ../../lib/cf-socket.h:55:25: error: expected ':', ',', ';', '}' or '__attribute__' before '.' token
       55 | #define sa_addr _sa_ex_u.addr
          |                         ^
  ```
  Ref: https://github.com/curl/curl/actions/runs/10738314933/job/29781644299?pr=14772#step:3:6115

Discovered while adding support for "unity" builds for autotools.

Required-by: curl#14922
Cherry-picked from curl#14815
Closes curl#14932
vszakats pushed a commit that referenced this pull request Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants