Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

[Clang] Make template instantiations respect pragma FENV_ACCESS state at point-of-definition
AbandonedPublic

Authored by zahiraam on Jun 13 2023, 7:46 AM.

Diff Detail

Event Timeline

zahiraam created this revision.Jun 13 2023, 7:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 13 2023, 7:46 AM
zahiraam requested review of this revision.Jun 13 2023, 7:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 13 2023, 7:46 AM

Has the test case been obfuscated on purpose?

Has the test case been obfuscated on purpose?

This is a smaller reproducible from creduce. Didn't know if it would be OK to have an include in a LIT test?

Has the test case been obfuscated on purpose?

This is a smaller reproducible from creduce. Didn't know if it would be OK to have an include in a LIT test?

Okay, not sure if we have a policy regarding that. Just looking at it, I don't think anyone understands what it's doing.

Has the test case been obfuscated on purpose?

This is a smaller reproducible from creduce. Didn't know if it would be OK to have an include in a LIT test?

Okay, not sure if we have a policy regarding that. Just looking at it, I don't think anyone understands what it's doing.

It's definitely related to the use of the pragma and the throw function. I could put the "fstream" header in an Inputs folder? I see an #include <stdio.h> in Analysis/z3/Inputs/MockZ3_solver_check.c.

Has the test case been obfuscated on purpose?

This is a smaller reproducible from creduce. Didn't know if it would be OK to have an include in a LIT test?

Okay, not sure if we have a policy regarding that. Just looking at it, I don't think anyone understands what it's doing.

It's definitely related to the use of the pragma and the throw function. I could put the "fstream" header in an Inputs folder? I see an #include <stdio.h> in Analysis/z3/Inputs/MockZ3_solver_check.c.

No, we don't want to include actual STL header content in the lit tests (we will mock up interfaces as needed, but they're usually uninteresting). It's worth noting that the original test case in your godbolt link has a quite different stack trace than your reduced example (they still end up in the same function though), so are there two different bugs? https://godbolt.org/z/Yaf5185cE

I spent some time reducing the test case down to:

void g(const char *);

template <class>
struct q {
  static void r() { g(""); }
};

#pragma STDC FENV_ACCESS ON
void a() { q<int>::r(); }

so there's nothing to do with throw expressions as the summary says. As best I can tell, this has something to do with template instantiation (making q no longer a template or calling g() directly both cause the issue to go away). I'd like to know a bit more about why the changes to the assertion are correct.

clang/lib/CodeGen/CodeGenFunction.cpp
161–164

Constructors and destructors are function declarations, so those no longer need an explicit check.

I'd guess we're not resetting the pragma state appropriately when instantiating templates. Expressions should be governed by the pragmas in scope at the point of definition, not at the point of instantiation.

I've opened an issue (https://github.com/llvm/llvm-project/issues/63063) where we hit the same assertion. There's a smaller test case, and we've done enough investigation that we understand the problem. I'd be happy to discuss over on the issue as well.

I've opened an issue (https://github.com/llvm/llvm-project/issues/63063) where we hit the same assertion. There's a smaller test case, and we've done enough investigation that we understand the problem. I'd be happy to discuss over on the issue as well.

Thanks. That sounds good!

hubert.reinterpretcast retitled this revision from [Clang] Fix assertion when pragma FENV_ACCESS is used with a throw function. to [Clang] Template instantiations fail to consistently respect pragma FENV_ACCESS state at point-of-definition.Jun 21 2023, 8:52 AM
hubert.reinterpretcast edited the summary of this revision. (Show Details)
hubert.reinterpretcast retitled this revision from [Clang] Template instantiations fail to consistently respect pragma FENV_ACCESS state at point-of-definition to [Clang] Make template instantiations respect pragma FENV_ACCESS state at point-of-definition.Jun 22 2023, 10:21 AM

Does https://reviews.llvm.org/D143241 solve the original problem here, or is there something deeper?

Does https://reviews.llvm.org/D143241 solve the original problem here, or is there something deeper?

It does not solve the problem, at least for my test case (linked in https://github.com/llvm/llvm-project/issues/63063) - we still hit the assertion.

Does https://reviews.llvm.org/D143241 solve the original problem here, or is there something deeper?

It does not solve the problem, at least for my test case (linked in https://github.com/llvm/llvm-project/issues/63063) - we still hit the assertion.

Sorry! I will not be able to work on this until about September! I haven't tried to reproduce the issue with the test case in the link above.

Does https://reviews.llvm.org/D143241 solve the original problem here, or is there something deeper?

It does not solve the problem, at least for my test case (linked in https://github.com/llvm/llvm-project/issues/63063) - we still hit the assertion.

Sorry! I will not be able to work on this until about September! I haven't tried to reproduce the issue with the test case in the link above.

We branch for the 17 release at the end of July, so I'm wondering whether there's anything we need to revert related to this? It looks like this assertion started firing in Clang 12.0.0 (https://cexplorer.testlabs.pro/z/Wh9o8W), so this doesn't seem to be a regression, but confirmation would be appreciated.

Does https://reviews.llvm.org/D143241 solve the original problem here, or is there something deeper?

It does not solve the problem, at least for my test case (linked in https://github.com/llvm/llvm-project/issues/63063) - we still hit the assertion.

Sorry! I will not be able to work on this until about September! I haven't tried to reproduce the issue with the test case in the link above.

We branch for the 17 release at the end of July, so I'm wondering whether there's anything we need to revert related to this? It looks like this assertion started firing in Clang 12.0.0 (https://cexplorer.testlabs.pro/z/Wh9o8W), so this doesn't seem to be a regression, but confirmation would be appreciated.

The code that is generating the assertion has been introduced by this patch: https://reviews.llvm.org/D80462
I can checkout that commit and see if the test case fails with it? Would that be a good experiment to do?

Does https://reviews.llvm.org/D143241 solve the original problem here, or is there something deeper?

It does not solve the problem, at least for my test case (linked in https://github.com/llvm/llvm-project/issues/63063) - we still hit the assertion.

Sorry! I will not be able to work on this until about September! I haven't tried to reproduce the issue with the test case in the link above.

We branch for the 17 release at the end of July, so I'm wondering whether there's anything we need to revert related to this? It looks like this assertion started firing in Clang 12.0.0 (https://cexplorer.testlabs.pro/z/Wh9o8W), so this doesn't seem to be a regression, but confirmation would be appreciated.

The code that is generating the assertion has been introduced by this patch: https://reviews.llvm.org/D80462
I can checkout that commit and see if the test case fails with it? Would that be a good experiment to do?

No need -- so long as we're all agreed that we haven't regressed anything between Clang 16 and Clang 17 here, that's all I'm really after.

Does https://reviews.llvm.org/D143241 solve the original problem here, or is there something deeper?

It does not solve the problem, at least for my test case (linked in https://github.com/llvm/llvm-project/issues/63063) - we still hit the assertion.

Sorry! I will not be able to work on this until about September! I haven't tried to reproduce the issue with the test case in the link above.

We branch for the 17 release at the end of July, so I'm wondering whether there's anything we need to revert related to this? It looks like this assertion started firing in Clang 12.0.0 (https://cexplorer.testlabs.pro/z/Wh9o8W), so this doesn't seem to be a regression, but confirmation would be appreciated.

The code that is generating the assertion has been introduced by this patch: https://reviews.llvm.org/D80462
I can checkout that commit and see if the test case fails with it? Would that be a good experiment to do?

No need -- so long as we're all agreed that we haven't regressed anything between Clang 16 and Clang 17 here, that's all I'm really after.

I verified that it's failing from clang12.0.0 through clang 16.0.0. I think this confirms that's not a regression between clang 16 and clang 17. Doesn't it?

Does https://reviews.llvm.org/D143241 solve the original problem here, or is there something deeper?

It does not solve the problem, at least for my test case (linked in https://github.com/llvm/llvm-project/issues/63063) - we still hit the assertion.

Sorry! I will not be able to work on this until about September! I haven't tried to reproduce the issue with the test case in the link above.

We branch for the 17 release at the end of July, so I'm wondering whether there's anything we need to revert related to this? It looks like this assertion started firing in Clang 12.0.0 (https://cexplorer.testlabs.pro/z/Wh9o8W), so this doesn't seem to be a regression, but confirmation would be appreciated.

The code that is generating the assertion has been introduced by this patch: https://reviews.llvm.org/D80462
I can checkout that commit and see if the test case fails with it? Would that be a good experiment to do?

No need -- so long as we're all agreed that we haven't regressed anything between Clang 16 and Clang 17 here, that's all I'm really after.

I verified that it's failing from clang12.0.0 through clang 16.0.0. I think this confirms that's not a regression between clang 16 and clang 17. Doesn't it?

Correct, it does, so nothing needs to be reverted.

zahiraam abandoned this revision.Sep 12 2023, 11:40 AM