This is an archive of the discontinued LLVM Phabricator instance.

[FPEnv] Initialization of C++ globals not strictfp aware
Needs ReviewPublic

Authored by kpn on Jun 4 2020, 10:58 AM.

Details

Summary

The rules of the strictfp attribute say that if it is used inside a function then it must also be in the function definition. Initialization of C++ globals breaks that rule. This patch corrects the failing test but doesn't attempt to correct it on other ABIs.

Error spotted with use of D68233.

Diff Detail

Event Timeline

kpn created this revision.Jun 4 2020, 10:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 4 2020, 10:58 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
kpn removed a subscriber: Restricted Project.Jun 4 2020, 11:00 AM
rjmccall added inline comments.Jun 5 2020, 3:12 PM
clang/lib/CodeGen/CodeGenFunction.h
4348

How is this set that we don't end up also setting it as a function attribute?

kpn marked an inline comment as done.Jun 8 2020, 11:00 AM
kpn added inline comments.
clang/lib/CodeGen/CodeGenFunction.h
4348

I admit I don't understand the question. Since the strictfp attribute is required on both definitions and calls then only one accessor function seems needed?

I don't get to spend as much time in clang proper as I might like.

kpn marked an inline comment as not done.Jun 8 2020, 11:00 AM
rjmccall added inline comments.Jun 8 2020, 11:32 AM
clang/lib/CodeGen/CodeGenFunction.h
4348

There is some code that's setting IsFPConstrained on the builder in the first place when building the global initialization function. We should be making sure that we add the IR attribute to the function under the same conditions.

The main place that sets IsFPConstrained is in CGF::StartFunction where it detects that we're emitting a FunctionDecl and checks whether the FunctionDecl usesFPIntrin(). That place also immediately adds the IR attribute. But there's another place, in CGF::SetFPModel, that sets it based on the global exception and rounding-mode settings, and that place doesn't add the attribute.

Both of these places look buggy, though, because they're both setting setIsFPConstrained(condition), which means that the second one to get called will overwrite the first rather than supplementing it. We should unify these places so that StartFunction is responsible for deciding whether we're using strictfp and, if so, both configures the builder properly and adds the attribute.