This is an archive of the discontinued LLVM Phabricator instance.

[clang][cli] Split DefaultAnyOf into a default value and ImpliedByAnyOf
ClosedPublic

Authored by jansvoboda11 on Nov 20 2020, 4:28 AM.

Details

Summary

This makes the options API composable, allows boolean flags to imply non-boolean values and makes the code more logical (IMO).

Diff Detail

Event Timeline

jansvoboda11 created this revision.Nov 20 2020, 4:28 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 20 2020, 4:28 AM
jansvoboda11 requested review of this revision.Nov 20 2020, 4:28 AM
jansvoboda11 removed a subscriber: dang.
jansvoboda11 edited the summary of this revision. (Show Details)Nov 20 2020, 4:36 AM

I like this direction; the logic in the .td files seems much cleaner. The MARSHALLING macro logic seems a bit harder to follow and there may be a bug, but I'm not sure. See comments inline.

clang/lib/Frontend/CompilerInvocation.cpp
3809–3811

This flip to the logic is interesting. I see it now matches the non-flag case; I can't remember if there was a subtle reason it was different before though.

4076–4080

I'm not sure this logic is quite right. It looks to me like if an option can very be implied, it will never be seriazed to -cc1, even if its current value is not an implied one.

Or have I understood the conditions under which IMPLIED_CHECK returns true?

IIUC, then this logic seems closer:

if (((FLAGS)&options::CC1Option) &&                                          \
    (ALWAYS_EMIT ||                                                          \
     (EXTRACTOR(this->KEYPATH) !=                                            \
      (IMPLIED_CHECK ? (DEFAULT_VALUE)                                       \
                     : (MERGER(DEFAULT_VALUE, IMPLIED_VALUE)))))) {          \
  DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, EXTRACTOR(this->KEYPATH));   \
}

It would be great to see tests in particular for a bitset (or similar) option where the merger does a union.

4087–4094

I'm not entirely sure if the comment applies here, since a bool option is simpler, but it would be good to have tests to demonstrate correct behaviour for options with the following scenarios:

  • option != default, it can be implied but the antecedents are false
  • option == default, it can be implied but the antecedents are false
  • option != default, it can be implied and the antecedents are true
  • option == default, it can be implied and the antecedents are true
4087–4094

(Maybe the tests already exist in tree; if so, please just point me at them)

dexonsmith requested changes to this revision.Nov 20 2020, 1:46 PM
This revision now requires changes to proceed.Nov 20 2020, 1:46 PM

Thanks for the feedback. I left some comments inline and will update the patch accordingly.

clang/lib/Frontend/CompilerInvocation.cpp
3809–3811

This makes more sense to me personally and I saw similar change in a later patch D84674. Let me know if you remember why it wasn't like that already.

4076–4080

IMPLIED_CHECK is a logical disjunction of the implying option keypaths. It evaluates to true whenever at least one of the implying keypaths evaluates to true.

I think I know what you're concerned about. Let me paraphrase it and check if my understanding is correct:
Suppose option a has default value of x, and flag b can imply the value of a to be y. If we have a command line -b -a=z, then -a=z would not be generated with the current logic: EXTRACTOR(this->KEYPATH) != DEFAULT_VALUE evaluates to true, but !(IMPLIED_CHECK) to false.

Your conditions gets close, but I think the ternary branches should be the other way around.

Here's a table exploring all cases:

IMPLIED_CHECK | EXTRACTOR(this->KEYPATH) == | SHOULD_EMIT
--------------+-----------------------------+-----------------------------------------
true          | IMPLIED_VALUE               | NO - emitting only the implying option is enough
true          | DEFAULT_VALUE               | YES - value explicitly specified (and it's DEFAULT_VALUE just by chance)
true          | ???                         | YES - value explicitly specified
false         | IMPLIED_VALUE               | YES - value explicitly specified (and it's IMPLIED_VALUE just by chance)
false         | DEFAULT_VALUE               | NO - default value handles this automatically
false         | ???                         | YES - value explicitly specified

I think this logic is what we're looking for:

if (((FLAGS)&options::CC1Option) &&                                          \
    (ALWAYS_EMIT ||                                                          \
     (EXTRACTOR(this->KEYPATH) !=                                            \
      ((IMPLIED_CHECK) ? (IMPLIED_VALUE) : (DEFAULT_VALUE))))) {             \
  DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, EXTRACTOR(this->KEYPATH));   \
}
4076–4080

It would be great to be able to test this logic even when no Clang options exercise it properly yet. Any ideas on that front?
Including a different Options.inc in CompilerInvocation.cpp for tests and re-linking the whole library seems wasteful.

4087–4094

The bool case works with the current logic, because there are only 2 options and typically, the implied value is negation of the default one.
I will change the logic to match what is discussed above to be consistent.

4087–4094

We don't test all cases currently, I'll fix that.

Add tests, fix condition in generator

dexonsmith added inline comments.Nov 30 2020, 7:01 AM
clang/lib/Frontend/CompilerInvocation.cpp
4076–4080

That'd be great, but I don't see a simple way. Can you find a flag that would exercise it properly? If so, I suggest converting just that one flag as part of this patch, and then you can test it.

dexonsmith accepted this revision.Nov 30 2020, 1:13 PM

This LGTM. Thanks again for the cleanup, I think the new syntax is more intuitive.

clang/lib/Frontend/CompilerInvocation.cpp
4076–4080

On second read, perhaps the tests you added are good enough for now, as long as you're careful to improve the test coverage when you land the patches with options that have interesting merger/bitset logic. I'll leave it up to you.

This revision is now accepted and ready to land.Nov 30 2020, 1:13 PM
This revision was landed with ongoing or failed builds.Dec 1 2020, 1:00 AM
This revision was automatically updated to reflect the committed changes.