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

Fall back to rustc's default value for crt-static #1266

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

madsmtm
Copy link
Contributor

@madsmtm madsmtm commented Nov 3, 2024

And parse more information from rustc, to prepare for doing more of #1249 and #268.

The net result is fairly small, it's that 37 targets (the full list is all targets with crt-static in their features table) now have -static passed when compiling outside a build script.

They should already have been passed when compiling in a build script, but that doesn't work, seemingly because of a bug in either rustc or Cargo that prevents the flag from propagating to CARGO_CFG_TARGET_FEATURE?

See also #1074, it sounds like this flag isn't that important?

@madsmtm madsmtm changed the title Fall back to rustc's default value for +crt-static/fn static_flag Fall back to rustc's default value for crt-static Nov 4, 2024
@madsmtm
Copy link
Contributor Author

madsmtm commented Nov 4, 2024

Opened rust-lang/cargo#14778 for the issue of Cargo not passing this in CARGO_CFG_TARGET_FEATURE.

There is not really a way that cc can detect whether we're hitting that bug, or if the flag is just absent because the user requested that it is absent, apart from maybe reading CARGO_ENCODED_RUSTFLAGS?

@madsmtm madsmtm force-pushed the rustc-crt-static branch 2 times, most recently from dc34c0f to 88cba35 Compare November 4, 2024 01:55
We do this by storing the default target features in `TargetInfo`.
Copy link
Collaborator

@NobodyXu NobodyXu left a comment

Choose a reason for hiding this comment

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

Thanks this mostly LGTM

Since there's a cargo bug, I think we should wait until upstream provides a clear answer to the issue.

match (name, value) {
("target_feature", Some(value)) => target_features.push(value.to_string()),
("target_family", Some(value)) => target_families.push(value.to_string()),
("target_endian", Some(value)) => target_endian = Some(value.to_string()),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit:

Shall we have a check for duplicate value, just in case rustc gives us some invalid output?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea

@@ -14,6 +14,7 @@ pub(crate) const LIST: &[(&str, TargetInfo<'static>)] = &[
env: "",
abi: "",
unversioned_llvm_target: "arm64-apple-macosx",
features: "aes,crc,dit,dotprod,dpb,dpb2,fcma,fhm,flagm,flagm2,fp16,frintts,jsconv,lor,lse,lse2,neon,paca,pacg,pan,pmuv3,ras,rcpc,rcpc2,rdm,sb,sha2,sha3,ssbs,v8.1a,v8.2a,v8.3a,v8.4a,vh",
Copy link
Collaborator

Choose a reason for hiding this comment

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

The generated file is getting larger and I'm slightly worried about the compile-time/binary size.

It probably would be fine for now and we should not hesitate adding more, if it eases maintenance burden and reduce hardcoded values.

Copy link
Contributor Author

@madsmtm madsmtm Nov 4, 2024

Choose a reason for hiding this comment

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

I suspect that the features here are going to be fairly important for runtime performance and correctness, but yeah, I do understand your worry.

I guess if we wanted to decrease binary size, we could parse them into a &[Feature] beforehand?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm...in terms of binary size it might be worse, there's a lot of small strings.

If we have small string optimization like docs.rs/compact-str then it might makes sense to do it.

Looking at the string, most are 3-5B, smaller than fat pointer, some are around ~10B, the longest is 20B.

Doing SSO would require quite some work and unsafe

Copy link
Collaborator

Choose a reason for hiding this comment

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

On the other hand, we could replace all this features with a boolean since all we need to know is if cry-static is included in the feature or not.

If we need more features later, then we can introduce them later, and probably use an enum which is more efficient than &str

Copy link
Collaborator

Choose a reason for hiding this comment

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

On the other hand, we could replace all this features with a boolean since all we need to know is if cry-static is included in the feature or not.

If we need more features later, then we can introduce them later, and probably use an enum which is more efficient than &str

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we want to reduce the binary size/compilation time, then I could think of two ideas:

First, split the target and TargetInfo into two arrays, this would help simplify the array, and might make binary search faster as more target can be in the cache.

For TargetInfo, we can store the string inline.

We can internalise the string into a one big &str and refer to it via index, we can simply use a u16 for base pointer.

The simplest impl is to use HashSet in gen-target-info to dedup them, and then concat them into one giant string, each terminated by \0 and assign them each a u16 based on the length within the giant string.

That will compress 16B &'static str on 64-bit platform to 2B, and should improve compile-time as well.

Though it looks like something out of scope for this ticket.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So for this PR, cc @madsmtm I just want you to replace features: &str with a crt_static: boolean, that would be good enough.

The rest of the idea should be discussed and completed in a different PR.

@NobodyXu
Copy link
Collaborator

NobodyXu commented Nov 5, 2024

Opened rust-lang/cargo#14778 for the issue of Cargo not passing this in CARGO_CFG_TARGET_FEATURE.

There is not really a way that cc can detect whether we're hitting that bug, or if the flag is just absent because the user requested that it is absent, apart from maybe reading CARGO_ENCODED_RUSTFLAGS?

cc @madsmtm given that the upstream probably won't fix CARGO_CFG_TARGET_FEATURE very soon, shall we read from CARGO_ENCODED_RUSTFLAGS instead?

@mrkajetanp
Copy link

shall we read from CARGO_ENCODED_RUSTFLAGS instead?

I should be able to post my PR for #1254 this week which introduces reading CARGO_ENCODED_RUSTFLAGS and adding cc flags based on that so it could come in handy for this?
Though in the general case I don't think it's a good idea to pass target features from there, instead target features should be read from CARGO_CFG_TARGET_FEATURE and managed separately. But if there's a bug like in this case, a stopgap solution could go there just fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants