-
Notifications
You must be signed in to change notification settings - Fork 450
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
base: main
Are you sure you want to change the base?
Conversation
f3bb04c
to
55c1d1c
Compare
rustc
's default value for +crt-static
/fn static_flag
rustc
's default value for crt-static
Opened rust-lang/cargo#14778 for the issue of Cargo not passing this in There is not really a way that |
dc34c0f
to
88cba35
Compare
88cba35
to
2a8113d
Compare
We do this by storing the default target features in `TargetInfo`.
2a8113d
to
54c1a4c
Compare
There was a problem hiding this 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()), |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
cc @madsmtm given that the upstream probably won't fix |
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? |
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 toCARGO_CFG_TARGET_FEATURE
?See also #1074, it sounds like this flag isn't that important?