[PATCH] D32195: Object: Remove ModuleSummaryIndexObjectFile class.

Fri Apr 28 13:27:52 PDT 2017

tejohnson added inline comments.

================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:63

+static llvm::cl::opt<bool> IgnoreEmptyThinLTOIndexFile(
+    "ignore-empty-index-file", llvm::cl::ZeroOrMore,
----------------
pcc wrote:
> tejohnson wrote:
> > pcc wrote:
> > > tejohnson wrote:
> > > > pcc wrote:
> > > > > pcc wrote:
> > > > > > tejohnson wrote:
> > > > > > > pcc wrote:
> > > > > > > > tejohnson wrote:
> > > > > > > > > Not sure llvm options are encouraged in clang (I only see a couple of cases currently).
> > > > > > > > > 
> > > > > > > > > Can you instead just move llvm::getModuleSummaryIndexForFile into BitcodeReader and avoid a lot of these changes?
> > > > > > > > The other clients do not need to be able to handle a null index (and I think some of them were crashing if they got one). I want to remove the possibility of the other clients getting a null index from this function.
> > > > > > > > 
> > > > > > > > The llvm::cl option is a pre-existing evil, this change isolates it to the only place that needs it and makes it easier to change to a proper clang option later.
> > > > > > > I only see one llvm::cl::opt that is linked into clang itself (the rest are used by various tools under clang/tools AFAICT. And I suspect that one snuck in - I believe I tried to add another one at some point and it was called out in a review, but I can't seem to find that now. 
> > > > > > > 
> > > > > > > Since the internal option is off by default, I don't understand the issue about other clients crashing if they got a null index. Presumably they crashed because it was null, which isn't expected without enabling the internal option.
> > > > > > > 
> > > > > > > Also, by moving llvm::getModuleSummaryIndexForFile into BitcodeReader you will avoid some of the churn/duplication in llvm (calls to getFileOrSTDIN). 
> > > > > > I agree that it is bad to have a cl::opt in clang, but what is even worse is sneaking one into clang by putting it in llvm instead, as it makes it harder to understand that the option is in fact a clang option. I think it was you who added this option, so I would ask you to remove it eventually, but I don't think it should block this change.
> > > > > > 
> > > > > > Regarding null, yes, the clients would crash if the internal option is enabled, which is why it shouldn't even be possible to enable it.
> > > > > > 
> > > > > > Regarding duplication, clients already have to open regular bitcode module files in order to read them, so I don't think it is a big deal.
> > > > > I think this is the review where you originally tried to add the option to clang: D28362
> > > > > 
> > > > > I think the right way to support this option would be to add a -cc1 flag to the frontend. If you like, I could add a FIXME to do that.
> > > > > I agree that it is bad to have a cl::opt in clang, but what is even worse is sneaking one into clang by putting it in llvm instead, as it makes it harder to understand that the option is in fact a clang option. I think it was you who added this option, so I would ask you to remove it eventually, but I don't think it should block this change.
> > > > 
> > > > I added this internal option to llvm, yes. I don't agree that it is a clang option, it is an llvm option that indicates the reader should not issue an error when we try to read the summary from an empty bitcode file.
> > > > 
> > > > Note that other clients that pass an empty index file to the reader will get an error (without enabling the option). Probably they aren't set up to handle the null output when the option is enabled though. Are you concerned that someone may pass an empty index file to one of the other client tools (e.g. llvm-lto) that read index files, that they will enable the internal option, and will get a crash rather than an error message? Because the empty summary will not be handled either way today. If we want to have other clients handle these empty summaries, we'll need to do some work there in either case, in which case I think it makes even more sense for this internal option to remain in llvm.
> > > > 
> > > > What part are you asking me to remove eventually? We need this for distributed builds,
> > > It may be appropriate for the functionality to live in llvm if we want other clients to handle it. But it should not be controlled by a cl::opt, because I would not expect a cl::opt to control the invariants of an API. If the client can accept files without summaries, it should signal that either by calling a different function or through a function argument.
> > > 
> > > I would prefer to leave the functionality in clang until/unless some other client needs it. As you said yourself in D28362, the feature is a temporary workaround, so I would hope that it gets removed sooner than it needs to be supported in some other client.
> > > 
> > > That said I guess I'd be fine moving it back to llvm if you still disagree,.
> > > It may be appropriate for the functionality to live in llvm if we want other clients to handle it. 
> > 
> > I'd argue that if someone is enabling this option in an attempt to get other clients not to error on empty index files then we should support that in those clients, in which case this needs to live in llvm anyway.
> > 
> > >But it should not be controlled by a cl::opt, because I would not expect a cl::opt to control the invariants of an API. If the client can accept files without summaries, it should signal that either by calling a different function or through a function argument.
> > 
> > Is the handling of 0-sized files an invariant of the API? (Ironically, the description of llvm::getModuleSummaryIndexForFile is that it will "return the module summary index object if found, or nullptr if not", which for a 0-sized file matches the description of behavior when the internal option is enabled.)
> > 
> > > As you said yourself in D28362, the feature is a temporary workaround,
> > 
> > You are referring to where I said "I am investigating whether this can be addressed in our build system, but that is a longer term fix and so this enables a workaround in the meantime." Unfortunately, my investigation found that handling this on the build system side would be very difficult. So the support will need to stay in some form.
> > I'd argue that if someone is enabling this option in an attempt to get other clients not to error on empty index files then we should support that in those clients, in which case this needs to live in llvm anyway.
> 
> I feel that this argument is backwards. If you start with the need to support some feature in a client then, fine, this could be in llvm, but it doesn't follow that the best way to support it is with a cl::opt.
> 
> E.g. suppose that some client wants to support this feature without the need to pass any command line arguments, and that may indeed be the way to go if you think this will become a permanent feature of the compiler. Then it would certainly not be appropriate to use a cl::opt to control the behaviour.
> 
> > Is the handling of 0-sized files an invariant of the API?
> 
> I think so, it affects the contract of what it can return and when.
> 
> > (Ironically, the description of llvm::getModuleSummaryIndexForFile is that it will "return the module summary index object if found, or nullptr if not", which for a 0-sized file matches the description of behavior when the internal option is enabled.)
> 
> We need to update that documentation at least.
> E.g. suppose that some client wants to support this feature without the need to pass any command line arguments, and that may indeed be the way to go if you think this will become a permanent feature of the compiler. Then it would certainly not be appropriate to use a cl::opt to control the behaviour.

Sure, but I wasn't necessarily advocating making it the default behavior. Just that other clients may want to support this mode as well.

> I think so, it affects the contract of what it can return and when.

Or simply document that the behavior when passed a 0-sized input file is dependent on the option?

> We need to update that documentation at least.

Yes. At one point that was likely correct in the case where it is provided a valid bitcode file without any summary. But now it looks like that just returns an Index that will be empty.

https://reviews.llvm.org/D32195