Remove no_mangle from new_session

[jyn514]

I ran into the following error while updating quiche a while back:

= note: /usr/bin/ld: target/debug/deps/libquiche-aebddac5b5b78042.rlib(quiche-aebddac5b5b78042.quiche.2dd0d882-cgu.7.rcgu.o): in function `new_session':
        /home/builder/.cargo/git/checkouts/quiche-3a3206f14f25c564/a0088a9/quiche/src/tls.rs:1010: multiple definition of `new_session'; target/debug/deps/libquiche-7702398d4219d80f.rlib(quiche-7702398d4219d80f.quiche.1f33b6b3-cgu.5.rcgu.o):/home/builder/.cargo/git/checkouts/quiche-3a3206f14f25c564/df3fac0/src/tls.rs:1010: first defined here

That's not a particularly helpful error - the issue was that that we had two versions of quiche in
the dependency tree. This removes the unnecessary no_mangle attribute from new_session -
it's only ever used as a function pointer, so it doesn't need to have a well-known name for the linker.

Eventually we might want to consider adding a links attribute, or consider separating the FFI bits
into a new quiche-sys crate, but I don't have time to drive that currently.

[ghedo]

AFAICT the no_mangle instance you are removing is the only one that is not gated behind the ffi feature flag (which Rust projects shouldn't ever need to enable), so I don't think we need the links change too?

I'm not necessarily against it, but I don't quite know what the full consequences of that change would be. We also have #1151 which does the same, but for different reasons.

[jyn514]

the no_mangle instance you are removing is the only one that is not gated behind the ffi feature flag (which Rust projects shouldn't ever need to enable)

I don't have a lot of context here, sorry - why would Rust projects never need to enable FFI?

Cargo unfortunately doesn't allow making this attribute conditional on feature flags ... I don't feel strongly about adding links if the no_mangle functions are feature gated, but it does seem like an unfortunate footgun.

I don't quite know what the full consequences of that change would be.

The only difference should be that cargo will no longer allow having multiple versions of quiche in the dependency tree, the same way you can't have multiple versions of boring-sys or jemalloc-sys. See https://doc.rust-lang.org/cargo/reference/resolver.html#links.

[jyn514]

We also have #1151 which does the same, but for different reasons.

I wonder if it makes sense to have a separate quiche-sys crate rather than a feature gate, and subvert the usual dependency order by having quiche-sys depend on the main library. That allows unconditionally putting links in quiche-sys without it impacting the main quiche library.

[ghedo]

I don't have a lot of context here, sorry - why would Rust projects never need to enable FFI?

Basically we expose an API that can be used as part of other non-Rust projects (e.g. curl, nginx) and we build a shared library (libquiche.so) for that purpose, but the actual implementation is gated behind a non-default feature (https://github.com/cloudflare/quiche/blob/master/quiche/src/lib.rs#L11369-L11370) to avoid exposing the API to Rust projects that should just use the Rust API instead.

So my thinking was that removing the only no_mangle that is outside the ffi feature (i.e. the one you are removing in this PR), should be enough to avoid the problem you run into and we wouldn't need the links change anymore.

I wonder if it makes sense to have a separate quiche-sys crate rather than a feature gate

I thought about that and we might do it in the future, but it would be a breaking change and notifying/updating all the projects that depend on this takes time and is kind of a pain, so I don't think it will happen any time soon.

[ghedo]

To clarify, the quiche-sys idea I had was to basically move the whole FFI API to that new crate and make it crate-type = ["staticlib", "cdylib"] so that the main quiche crate wouldn't build the libquiche.so library anymore.

[jyn514]

Got it - I do think it's worth working towards that quiche-sys crate, but I don't have time to drive that right now. I changed this to only remove the no_mangle attribute for now.

[ghedo]

Merged, thanks @jyn514!