-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
The default values for TargetOption violate the target consistency check #133459
Comments
The consistency check is reasonable, you don't typically need PIC if you don't have dynamic linking (or PIEs) The defaults are also reasonable, all of them use safe choices. |
I think JSON-defined targets should pass the consistency check (see #133409). And, unsurprisingly, most JSON-defined targets leave those values at their default. But also, changing them would change the behavior of those JSON targets, wouldn't it? |
This sounds like a case where a "warning" vs. "error" distinction would be sufficient to solve the problem? That is, it would be reasonable for all-defaults to be a configuration which is accepted with a warning. |
I think keeping an error and forcing folk to make a concious choice to pick sth is good. Changing the defaults could cause a lot of problems, many unnoticeable at build time (link time or runtime would show issues). If we want to change the default, we should make sure that anyone relying on the defaults gets informed somehow, which I think is only possible by renaming all fields in a consistency set |
For json targets, this is currently not an error. My PR makes it so that this is an error only for builtin targets. But that makes me wonder which other checks should also be treated like that.
|
I'm confused, why would we default to PIC but not PIE? |
TargetOption
defaults torelocation_model: Pic
anddynamic_linking: false
andposition_independent_executables: false
. However, our consistency check that this is an invalid combination of options.So it seems we should either fix our defaults, or the consistency check is a bit overzealous?
This check was added in #100537, Cc @petrochenkov @oli-obk
The text was updated successfully, but these errors were encountered: