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

[sameerarora101]

Summary

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

[jhenderson]

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]

In D84209#2163675, @jhenderson wrote:

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!

[sameerarora101]

Adding docs

[sameerarora101]

Rebasing

[MaskRay]

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.

[jhenderson]

In D84209#2164501, @sameerarora101 wrote:

In D84209#2163675, @jhenderson wrote:

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.

[sameerarora101]

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

[sameerarora101]

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?

[jhenderson]

In D84209#2167919, @sameerarora101 wrote:

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.

[sameerarora101]

In D84209#2168725, @jhenderson wrote:

In D84209#2167919, @sameerarora101 wrote:

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]

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.

[sameerarora101]

Changing default behavior to Deterministic and adding -U flag

[sameerarora101]

Updating the docs for -U flag

[jhenderson]

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)

172

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]

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?

172

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]

checkCommandLineOptions -> parseCommandLine

[sameerarora101]

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]

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]

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

25

Added the config, thanks.

[sameerarora101]

Add Config struct.

[jhenderson]

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

55

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

102

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

179–180

Consider this instead, combined with the default initialiser:

else if (NonDeterminisitcOption)
  C.Deterministic = false;

[sameerarora101]

Config C -> const Config &C

[jhenderson]

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

[smeenai]

LGTM

[sameerarora101]

Rebase.