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

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

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

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

wants to merge 4 commits into from
+48 −46

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Sep 16, 2024 •
edited
Loading

  • 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.

@vszakats vszakats mentioned this pull request Sep 16, 2024
Open
5 tasks
@vszakats vszakats requested review from icing and bagder and removed request for icing September 16, 2024 13:39
@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 •
edited
Loading

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

@talregev talregev mentioned this pull request Sep 16, 2024
Open
4 tasks
@vszakats
am: unity fix local macro clash with BearSSL header
7fd7129
@vszakats
am: unity fix local macro clashing with OmniOS
4f38bf9 
```
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
am: unity fix msh3 symbol clash
fe3a67e
@vszakats
am: unity fix quiche symbol clash
3900d68
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bagder bagder Awaiting requested review from bagder

@icing icing Awaiting requested review from icing

Assignees
No one assigned
Labels
build
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vszakats @bagder @dfandrich