This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by jansvoboda11 on Jul 6 2020, 4:26 AM.

Diff Detail

Event Timeline

dang created this revision.Jul 6 2020, 4:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 6 2020, 4:26 AM
dang updated this revision to Diff 276466.Jul 8 2020, 9:37 AM

Rebase on top of latest changes

Bigcheese added inline comments.Aug 19 2020, 5:52 PM
clang/lib/Frontend/CompilerInvocation.cpp
3931

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 commandeered this revision.Nov 16 2020, 1:49 AM
jansvoboda11 added a reviewer: dang.
jansvoboda11 added a subscriber: jansvoboda11.

Taking over this patch as Daniel is no longer involved.

Rebase and add documenting possible reference lifetime extension issue

Addressed review feedback. WDYT @Bigcheese?

clang/lib/Frontend/CompilerInvocation.cpp
3931

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

dexonsmith added inline comments.Nov 18 2020, 11:34 AM
clang/lib/Frontend/CompilerInvocation.cpp
3931

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));
3931

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 added inline comments.Nov 18 2020, 11:44 AM
clang/lib/Frontend/CompilerInvocation.cpp
3931

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 requested changes to this revision.Nov 20 2020, 3:44 PM

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.

This revision now requires changes to proceed.Nov 20 2020, 3:44 PM
jansvoboda11 added inline comments.Nov 23 2020, 5:42 AM
clang/lib/Frontend/CompilerInvocation.cpp
3931

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?

3931

Yes, we could simplify this for booleans.

Bigcheese accepted this revision.Nov 24 2020, 8:53 AM
Bigcheese added inline comments.
clang/lib/Frontend/CompilerInvocation.cpp
3931

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 added inline comments.Nov 24 2020, 9:06 AM
clang/lib/Frontend/CompilerInvocation.cpp
3931

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.

Implement review feedback

Herald added a project: Restricted Project. · View Herald TranscriptNov 26 2020, 5:12 AM
jansvoboda11 retitled this revision from Factor out call to EXTRACTOR in generateCC1CommandLine to [clang][cli] Factor out call to EXTRACTOR in generateCC1CommandLine (NFC).Nov 26 2020, 5:12 AM

Remove lifetime comment

Undo whitespace change

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