[clang][cli] Factor out call to EXTRACTOR in generateCC1CommandLine (NFC)

[dang]

Rebase on top of latest changes

[Bigcheese]

clang/lib/Frontend/CompilerInvocation.cpp

4028

Will this ever have an issue with lifetime? I can see various values for EXTRACTOR causing issues here. https://abseil.io/tips/107

It would be good to at least document somewhere the restrictions on EXTRACTOR.

[jansvoboda11]

Taking over this patch as Daniel is no longer involved.

[jansvoboda11]

Rebase and add documenting possible reference lifetime extension issue

[jansvoboda11]

Addressed review feedback. WDYT @Bigcheese?

clang/lib/Frontend/CompilerInvocation.cpp

4028

Mentioned the reference lifetime extension in a comment near extractor definitions.

[jansvoboda11]

Rebase

[dexonsmith]

clang/lib/Frontend/CompilerInvocation.cpp

4028

It might be safer to refactor as:

// Capture the extracted value as a lambda argument to avoid potential
// lifetime extension issues.
[&](const auto &Extracted) {
  if (ALWAYS_EMIT || Extracted != (DEFAULT_VALUE))
    DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, Extracted);
}(EXTRACTOR(this->KEYPATH));

4040

IIUC, then we can do this more simply as:

bool Extracted = EXTRACTOR(this->KEYPATH);

That might clarify why we don't have lifetime extension concerns here.

[dexonsmith]

clang/lib/Frontend/CompilerInvocation.cpp

4028

Might be even better to avoid the generic lambda:

// Capture the extracted value as a lambda argument to avoid potential
// lifetime extension issues.
using ExtractedType =
    std::remove_const_t<std::remove_reference_t<
        decltype(EXTRACTOR(this->KEYPATH))>>
[&](const ExtractedType &Extracted) {
  if (ALWAYS_EMIT || Extracted != (DEFAULT_VALUE))
    DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, Extracted);
}(EXTRACTOR(this->KEYPATH));

(since generic vs. non-generic could affect compile-time of CompilerInvocation.cpp given how many instances there will be).

[dexonsmith]

Marking as "requested changes" to get this off my queue, but if my lambda suggestion doesn't work well for some reason (maybe it significantly increases compile-time), I'm open to sticking to the documentation @Bigcheese suggested.

[jansvoboda11]

clang/lib/Frontend/CompilerInvocation.cpp

4028

Thanks for the suggestions @dexonsmith. I'm having trouble writing a test case where the lambda workaround produces a different result than const auto & variable.
@Bigcheese, could you show a concrete example of an extractor that causes issues so I can test it out?

4040

Yes, we could simplify this for booleans.

[Bigcheese]

clang/lib/Frontend/CompilerInvocation.cpp

4028

I think I was confused about when this can happen. The const auto & will never cause a conversion that would prevent lifetime extension. The only place this would break is if the copy constructor of the type is rather broken, which isn't a reasonable case to support.

[Bigcheese]

clang/lib/Frontend/CompilerInvocation.cpp

4028

Now I remember what the issue was, it's if the EXTRACTOR itself creates an owning temporary and converts back to a reference type: https://godbolt.org/z/xsvh4f

This is kind of weird to do, the EXTRACTOR would need to take an owning type with an implicit conversion from the type of KEYPATH, and then return a reference type. I'm not sure how realistic that situation is, but it seems fine to defend against with Duncan's suggestion.

[jansvoboda11]

Implement review feedback

[jansvoboda11]

Remove lifetime comment

[jansvoboda11]

Undo whitespace change

[dexonsmith]

LGTM!

clang/lib/Frontend/CompilerInvocation.cpp

 //===- CompilerInvocation.cpp ---------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 

 static T mergeForwardValue(T KeyPath, U Value) {
   return Value;
 }
 
 template <typename T, typename U> static T mergeMaskValue(T KeyPath, U Value) {
   return KeyPath | Value;
 }
 
+// The value returned by an extractor is stored as a constant reference.
+// Watch out for missed reference lifetime extension.
+
 template <typename T> static T extractForwardValue(T KeyPath) {
   return KeyPath;
 }
 
 template <typename T, typename U, U Value>
 static T extractMaskValue(T KeyPath) {
   return KeyPath & Value;
 }
#include "clang/Basic/LangOptions.def"
   if (!SanHash.empty())
     code = hash_combine(code, SanHash.Mask);
 
   return llvm::APInt(64, code).toString(36, /*Signed=*/false);
 }
 
 void CompilerInvocation::generateCC1CommandLine(
     SmallVectorImpl<const char *> &Args, StringAllocator SA) const {
+  // Capture the extracted value as a lambda argument to avoid potential issues
+  // with lifetime extension of the reference.
 #define OPTION_WITH_MARSHALLING(                                               \
     PREFIX_TYPE, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,        \
     HELPTEXT, METAVAR, VALUES, SPELLING, ALWAYS_EMIT, KEYPATH, DEFAULT_VALUE,  \
     NORMALIZER, DENORMALIZER, MERGER, EXTRACTOR, TABLE_INDEX)                  \
-  if (((FLAGS) & options::CC1Option) &&                                        \
-      (ALWAYS_EMIT || EXTRACTOR(this->KEYPATH) != (DEFAULT_VALUE))) {          \
-    DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, EXTRACTOR(this->KEYPATH));   \
+  if ((FLAGS)&options::CC1Option) {                                            \
+    [&](const auto &Extracted) {                                               \
+      if (ALWAYS_EMIT || Extracted != (DEFAULT_VALUE))                         \
+        DENORMALIZER(Args, SPELLING, SA, TABLE_INDEX, Extracted);              \
+    }(EXTRACTOR(this->KEYPATH));                                               \
   }
 
 #define OPTION_WITH_MARSHALLING_BOOLEAN(                                       \
     PREFIX_TYPE, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,        \
     HELPTEXT, METAVAR, VALUES, SPELLING, ALWAYS_EMIT, KEYPATH, DEFAULT_VALUE,  \
     NORMALIZER, DENORMALIZER, MERGER, EXTRACTOR, TABLE_INDEX, NEG_ID,          \
     NEG_SPELLING)                                                              \
-  if (((FLAGS)&options::CC1Option) &&                                          \
-      (ALWAYS_EMIT || EXTRACTOR(this->KEYPATH) != DEFAULT_VALUE)) {            \
-    DENORMALIZER(Args, SPELLING, NEG_SPELLING, SA, TABLE_INDEX,                \
-                 EXTRACTOR(this->KEYPATH));                                    \
+  if ((FLAGS)&options::CC1Option) {                                            \
+    bool Extracted = EXTRACTOR(this->KEYPATH);                                 \
+    if (ALWAYS_EMIT || Extracted != (DEFAULT_VALUE))                           \
+      DENORMALIZER(Args, SPELLING, NEG_SPELLING, SA, TABLE_INDEX, Extracted);  \
   }
 
 #include "clang/Driver/Options.inc"
 #undef OPTION_WITH_MARSHALLING_BOOLEAN
 #undef OPTION_WITH_MARSHALLING
 }
 
 IntrusiveRefCntPtr<llvm::vfs::FileSystem>

llvm/unittests/Option/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Option
   Support
   )
 
 set(LLVM_TARGET_DEFINITIONS Opts.td)
-
 tablegen(LLVM Opts.inc -gen-opt-parser-defs)
+
+set(LLVM_TARGET_DEFINITIONS LifetimeOpts.td)
+tablegen(LLVM LifetimeOpts.inc -gen-opt-parser-defs)
+
 add_public_tablegen_target(OptsTestTableGen)
 
 add_llvm_unittest(OptionTests
   OptionParsingTest.cpp
   OptionMarshallingTest.cpp
   )

llvm/unittests/Option/LifetimeOpts.td

+include "llvm/Option/OptParser.td"
+
+def lifetime_test : Flag<["-"], "lifetime-test">,
+  MarshallingInfoFlag<"FlagValue">,
+  ValueExtractor<"extractorRequiringLifetimeExtension">;