Page MenuHomePhabricator

[llvm-libtool-darwin] Add support for -D and -U option
ClosedPublic

Authored by sameerarora101 on Jul 20 2020, 4:16 PM.

Details

Summary

-D allows for using zero for timestamps and UIDs/GIDs.
-U allows for using actual timestamps and UIDs/GIDs.
Depends on D84206.

Diff Detail

Event Timeline

sameerarora101 created this revision.Jul 20 2020, 4:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 20 2020, 4:16 PM

Documentation for this option? Also, you've changed the default behaviour of the tool from being deterministic to being non-deterministic. Are you sure that's what you want?

sameerarora101 added a comment.EditedJul 21 2020, 9:11 AM

Documentation for this option? Also, you've changed the default behaviour of the tool from being deterministic to being non-deterministic. Are you sure that's what you want?

Would changing the default behavior to being non-deterministic be a problem? This, non-deterministic behavior, is the default behavior of cctools' libtool and so I thought of replicating that here.
I have added the docs now, thanks!

In binutils, there is a configure-time option --enable-deterministic-archives. For llvm-objcopy and llvm-ar, we find that deterministic builds tend to make more sense so we simply default to determinism. There probably should be a survey whether cctool libtool users really want the default non-deterministic behavior.

Documentation for this option? Also, you've changed the default behaviour of the tool from being deterministic to being non-deterministic. Are you sure that's what you want?

Would changing the default behavior to being non-deterministic be a problem? This, non-deterministic behavior, is the default behavior of cctools' libtool and so I thought of replicating that here.
I have added the docs now, thanks!

I don't know whether it would be a problem or not, but it does make comparing archives harder. That being said, in our downstream llvm-ar, we've had to override the default to use non-deterministic timestamps, because our build system relies on the timestamps being accurate, so that might be evidence that changing it is good. I don't know if it's important to people outside our environment though - there aren't lots of people complaining about the current upstream llvm-ar behaviour, so matching that might make more sense. I don't really have an opinion either way. I'm happy for users of the tool to provide guidance.

Thanks, I'll wait for others to respond too. Maybe @alexshap and @smeenai can comment on the right behavior here?

It seems like cctools' libtool does not have an explicit flag for indicating non-deterministic behavior. Thus, if I set Deterministic to true by default, I would need to implement another flag (something like -U) to specify non-deterministic behavior for llvm-libtool. This would then be a slight deviation from libtool's behavior. What do you guys think?

It seems like cctools' libtool does not have an explicit flag for indicating non-deterministic behavior. Thus, if I set Deterministic to true by default, I would need to implement another flag (something like -U) to specify non-deterministic behavior for llvm-libtool. This would then be a slight deviation from libtool's behavior. What do you guys think?

I think if we have Deterministic set to a different default value to libtool, we should definitely provide a -U option to allow a fallback, in case there are people who need that behaviour.

It seems like cctools' libtool does not have an explicit flag for indicating non-deterministic behavior. Thus, if I set Deterministic to true by default, I would need to implement another flag (something like -U) to specify non-deterministic behavior for llvm-libtool. This would then be a slight deviation from libtool's behavior. What do you guys think?

I think if we have Deterministic set to a different default value to libtool, we should definitely provide a -U option to allow a fallback, in case there are people who need that behaviour.

I see. Would we still need the -D option in that case?

sameerarora101 added a comment.EditedJul 23 2020, 8:35 AM

I have updated the diff below to add support for both -D and -U options. Although, the -D flag is not necessary, we perhaps need it to align the behavior with cctools' libtool? I can update it by removing the D flag if people prefer that? Thanks.

Changing default behavior to Deterministic and adding -U flag

Updating the docs for -U flag

sameerarora101 retitled this revision from [llvm-libtool-darwin] Add support for -D option to [llvm-libtool-darwin] Add support for -D and -U option.Jul 23 2020, 8:42 AM
sameerarora101 edited the summary of this revision. (Show Details)
jhenderson added inline comments.Jul 28 2020, 1:09 AM
llvm/test/tools/llvm-libtool-darwin/deterministic-library.test
2–3

Change "timestamps/etc." to simply "timestamps". You're next paragraph explains the bit about other fields, so the "etc" is somewhat misleading/confusing.

Also, combine these into one sentence and add a couple of missing "the"s: "This test checks that timestamps are set to 0 by default or when the -D option is specified, and that they are preserved when the -U option is specified."

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
25

I'm not particularly a fan of adding a global variable for this. Are there going to be other configuration options? If so, I'd recommend creating a Config class/struct, that can be created and passed around instead.

(Also, missing trailing full stop in comment)

186

This function does more than just check the options - it also sets behaviour, based on options that are parsed. I think you need to rename the function to clarify. If you add a Config class, perhaps it could become: static Expected<Config> parseCommandline() instead, and also do the cl::ParseCommandLineOptions call. What do you think?

sameerarora101 marked 3 inline comments as done.Jul 28 2020, 6:56 AM
sameerarora101 added inline comments.
llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
25

For now, there aren't any other configuration options that vary depending on the flags passed in on the command line. However, I can still wrap this in a Config class/struct just in case one is needed in the future? What do you think?

186

Yup, I agree on the name and cl::ParseCommandLineOptions call, I have changed it accordingly now. (I can change the signature too if we decide on adding the Config class - please take a look at my comment above for that).

sameerarora101 marked 2 inline comments as done.Jul 28 2020, 6:56 AM

checkCommandLineOptions -> parseCommandLine

sameerarora101 added inline comments.Jul 28 2020, 7:08 AM
llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
25

By "For now" I am referring to the period until we add full support for creating static libraries. ( I am not completely sure yet about the dynamic case)

jhenderson added inline comments.Jul 29 2020, 3:37 AM
llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
25

I'm talking about in the future. I'm guessing there will be other configuration options added in due course, but you have a better idea of this than me. If there are, I'd recommend my suggested approach.

sameerarora101 marked an inline comment as done.Jul 29 2020, 7:00 AM
sameerarora101 added inline comments.
llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
25

Added the config, thanks.

sameerarora101 marked an inline comment as done.

Add Config struct.

jhenderson added inline comments.Jul 31 2020, 12:49 AM
llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
59

How about Deterministic = true (or false, whichever is the preferred default)?

114

Config is likely to grow, so it probably is best to pass it as a const &, here and everywhere else.

193–194

Consider this instead, combined with the default initialiser:

else if (NonDeterminisitcOption)
  C.Deterministic = false;
sameerarora101 marked 3 inline comments as done.Jul 31 2020, 7:42 AM

Config C -> const Config &C

jhenderson accepted this revision.Aug 3 2020, 12:03 AM

LGTM, but as always, please get somebody else with relevant experience of the existing tool to sign off.

This revision is now accepted and ready to land.Aug 3 2020, 12:03 AM
smeenai accepted this revision.Aug 4 2020, 5:32 PM

LGTM

This revision was landed with ongoing or failed builds.Aug 7 2020, 2:47 PM
This revision was automatically updated to reflect the committed changes.