[PATCH] D58497: Clear the KnownModules cache if the preprocessor is going away

Sun Mar 31 18:47:25 PDT 2019

nemanjai added a comment.

In D58497#1449306 <https://reviews.llvm.org/D58497#1449306>, @dblaikie wrote:

> In D58497#1449243 <https://reviews.llvm.org/D58497#1449243>, @nemanjai wrote:
>
> > Ping.
>
>
> Unfortunately Richard Smith is out for a few weeks at the moment, so might take a little bit before he can get to this.
>
> It's odd to me that this lacks a test case - but you mention it's shown up on buildbots? Does it reproduce consistently there? Under what conditions (which buildbots/configurations show this - are they permanently failing because of this?)?
>
> A test case, if at all possible, would be super helpful.

The failure this causes always shows up in the `Modules/builtins.m` test (at least in my experience). It is far from predictable and it does not consistently reproduce on any build bot. It occasionally shows up and slight perturbations in the source make it go away.
Honestly, I don't find this to be all that surprising. Using memory after freeing it has inherently unpredictable behaviour. There are certain toolchains that will diagnose freeing the same memory twice, but that's not the case here - we just happen to use it after freeing it.

> 
> 
>> If there are no objections in the next week or so, I'll commit this and it can be reviewed post-commit.
> 
> That's generally not considered acceptable practice - if something is sent for review it's because it needs review & time doesn't change that. (there are some exceptions to this - some folks send things out for "hey, anyone got other ideas on this, otherwise I think it's fine" sort of thing)

I am really sorry about how this came across. I understand that given the context, this could quite reasonably be interpreted as me stating "I don't want to wait any longer, so I'm just going to commit this." That was not at all my intention. I merely meant to state that I don't believe this to be in any way controversial. I have shown quite clearly in my email that `KnownModules` will have pointers to data that the `Preprocessor` owns. If the existing `Preprocessor` shared pointer is the last reference, it will obviously be deleted now that we're reassigning to it. Thereby, we are deleting the `Preprocessor` which will delete all the data it owns and we are keeping `KnownModules` alive (with cached pointers to data that is being deleted). There is no situation I can think of in which it is reasonable to keep pointers to deleted data. If I came across an issue of this nature - clearly undefined behaviour - in the PPC back end where I spend most of my time, I'd probably not post for review as a fix is clearly in order. But since I am not intimately familiar with this code, I thought I'd get another opinion on the fix by sending an email to the dev list and posting on Phabricator.

All that being said, it sounds like there is an objection to me committing this so I certainly won't proceed without an approval on this review. If you or anyone else can offer a suggestion on how I might come up with a test case for this - or perhaps an alternative fix for this issue, I am more than happy to incorporate your suggestions.

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58497/new/

https://reviews.llvm.org/D58497