[PATCH] D139713: [Sema] Fix crash when evaluating nested call with value-dependent arg

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

In D139713#3989709 <https://reviews.llvm.org/D139713#3989709>, @Pierre-vh wrote:

> In D139713#3989071 <https://reviews.llvm.org/D139713#3989071>, @shafik wrote:
>
>> If I am reading the code correctly it looks like if the call to `(*I)->isValueDependent()` is true then the temporary will not be created and therefore we should not be attempting to access the slot.
>>
>> If this is the case then maybe the checking in `EvaluateWithSubstitution(...)` needs to be more carefully done?
>>
>> I am not familiar with this code but I don't know if you analysis provides a convincing case the assert should be removed.
>
> Indeed, this only happens when isValueDependent returns true.
>
> I am also not familiar with the code, so I just decided to propose a quick fix to get the discussion started; I certainly don't mind changing the nature of the fix if we agree it should be fixed differently.
> For instance, we could also make the "getParam" call faillible by adding some "tryGetParam" variant that doesn't have the assert, or by passing some optional boolean to indicate it's acceptable to have the key present in the map with a different version.
>
> My initial reasoning was that if the assert can be broken by legitimate code, then it shouldn't be there

It looks like the original commit that added `getTemporary(...)` which was 4e2698ca9e49e that this invariant held but the later commit which added `getParamSlot(...)`  which is f7f2e4261a98b <https://reviews.llvm.org/rGf7f2e4261a98b2da519d58e7f6794b013cda7a4b> broke the invariant.

So I think that looks like an error and any fix should maintain the invariant unless strongly motivated reasoning can be put forward to remove it but I am not totally sure.

CC @rsmith @ahatanak who are the authors of the two commits mentioned above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139713