Page MenuHomePhabricator

Fix stack address builtin for negative numbers
ClosedPublic

Authored by zoecarver on Aug 27 2019, 3:49 PM.

Details

Summary

This patch checks that an argument passed to either of the builtins __builtin_frame_address or __builtin_return_address is at least 0. This patch resolves the issue where these builtins would cause an infinite loop ( 25497 ).

Event Timeline

zoecarver created this revision.Aug 27 2019, 3:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 27 2019, 3:49 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
zoecarver marked an inline comment as done.
zoecarver added inline comments.
clang/test/Sema/builtin-stackaddress.c
10

For some reason lit was still erroring here.

We usually prefer to generate error messages for incorrect parameters to builtins in SemaChecking.cpp.

I don't think there's really an infinite loop here. The parameter to __builtin_frame_address is an unsigned integer; values greater than 0x7FFFFFFU aren't special. The reason it takes a very long time is that we try to unroll the implied loop; generating an object file with four billion load instructions takes essentially forever. We could impose a smaller limit, but it would be sort of arbitrary.

Thanks for the explanation. Should I then not try to "fix" the issue? Or should I update sema checking?

In the context of __builtin_frame_address, an arbitrary limit is probably okay. Maybe something like 0xFFFF, which is larger than anyone would realistically use, but doesn't take a crazy amount of time to compile.

Jim added a subscriber: Jim.Dec 15 2019, 11:16 PM
Jim added a comment.Dec 16 2019, 1:53 AM

I think this checking should be implemented in lib/Sema/SemaChecking.cpp.

@Jim Thanks, I'll move it.

  • move verification to SemaChecking
This revision is now accepted and ready to land.Feb 24 2020, 12:49 PM
This revision is now accepted and ready to land.Feb 24 2020, 2:37 PM
zoecarver updated this revision to Diff 246486.Feb 25 2020, 9:06 AM
  • Error if SemaBuiltinConstantArgRange returns true, not false

I was using SemaBuiltinConstantArgRange incorrectly. I was returning an error for !SemaBuiltinConstantArgRange instead of SemaBuiltinConstantArgRange. Also, I only modified the fail tests so I wasn't able to catch the error. Updated and am running both the Sema and CodeGen tests now. Should fix the error.

zoecarver updated this revision to Diff 246488.Feb 25 2020, 9:09 AM
  • diff from master (arg phab...)
This revision was automatically updated to reflect the committed changes.