Page MenuHomePhabricator

[clang] Fix copy constructor of CompilerInvocation
ClosedPublic

Authored by jansvoboda11 on Mar 30 2021, 2:05 AM.

Details

Summary

The CompilerInvocationBase class factors out members of CompilerInvocation that need special handling (initialization or copy constructor), so that CompilerInvocation can be implemented as a simple value object.

Currently, the AnalyzerOpts member of CompilerInvocation violates that setup. This patch extracts the member to CompilerInvocationBase and handles it in the copy constructor the same way other it handles other members.

Diff Detail

Event Timeline

jansvoboda11 requested review of this revision.Mar 30 2021, 2:05 AM
jansvoboda11 created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMar 30 2021, 2:05 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I'm not sure a deep copy is entirely sound, due to odd ownership rules in "remapped buffers" (I forget the subclass that includes those). A few months ago I got most of the way to deleting those (lifting the logic up into the clang tooling that needed it)... maybe we need to push that work over the line before this is safe... or if this is in fact safe, can you explain why?

BTW, I may be responsible for the not-very-descriptive naming of CompilerInvocationBase... I could imagine new options coming along that get added to the wrong place in the future as well. Any thoughts on a good name to use instead?

clang/include/clang/Frontend/CompilerInvocation.h
86

I wonder if it would be better to signify this is a deep copy by naming the function, such as by calling it clone(). WDYT?

I'm not sure a deep copy is entirely sound, due to odd ownership rules in "remapped buffers" (I forget the subclass that includes those). A few months ago I got most of the way to deleting those (lifting the logic up into the clang tooling that needed it)... maybe we need to push that work over the line before this is safe... or if this is in fact safe, can you explain why?

Ah, I didn't notice those. I think you may be referring to PreprocessorOptions::RemappedFileBuffers. I agree that the raw MemoryBuffer pointers don't play well with the concept of deep copy, so getting that sorted would be good.

How about we split this patch into two?

  1. Changes to AnalyzerOptions. They make the current implementation of deep copy constructor more correct, so we can commit them right away.
  2. Addition of copy assignment. It increases the visibility/surface area of the deep copy operation, so we can commit that when RemappedFileBuffers get fixed. In the meantime, I can work around its absence in my next patch.

WDYT?

Note: I looked at the members of CompilerInvocation and noticed one other field that's a reference type: PreprocessorOptions::FailedModules, which acts as a shared state, not so much as an actual option. I don't think it creates lifetime issues like RemappedFileBuffers. Although sharing it across deep copies of CompilerInvocation may be a bit unexpected, that seems to be the current invariant. Extracting that out of PreprocessorOptions would be nice though.

BTW, I may be responsible for the not-very-descriptive naming of CompilerInvocationBase... I could imagine new options coming along that get added to the wrong place in the future as well. Any thoughts on a good name to use instead?

We could make the split obvious by renaming CompilerInvocationBase to CompilerInvocationBaseReferenceType, renaming CompilerInvocation to CompilerInvocationBaseValueType and creating new CompilerInvocation class that inherits from both. I'm not sure if we have any precedent for this pattern. I'll look around.

clang/include/clang/Frontend/CompilerInvocation.h
86

We can be more explicit about that. In that case, it would probably make sense to delete the copy constructor to make clone() the only way to get a deep copy.

Ping.

Thanks for the ping; I'd completely missed your follow-up. Just fixing AnalyzerOptions for now sounds great.

Remove copy-assignment changes

jansvoboda11 retitled this revision from [clang][invocation] Fix copy constructor, add copy assignment to CompilerInvocation to [clang][invocation] Fix copy constructor of CompilerInvocation.Apr 13 2021, 8:15 AM
jansvoboda11 edited the summary of this revision. (Show Details)
This revision is now accepted and ready to land.Apr 13 2021, 10:32 AM
jansvoboda11 retitled this revision from [clang][invocation] Fix copy constructor of CompilerInvocation to [clang] Fix copy constructor of CompilerInvocation.Apr 14 2021, 12:12 AM
This revision was landed with ongoing or failed builds.Apr 14 2021, 12:13 AM
This revision was automatically updated to reflect the committed changes.