Page MenuHomePhabricator

[X86] Initialize TargetOptions::StackProtectorGuardOffset member to its default value
ClosedPublic

Authored by LemonBoy on Apr 6 2021, 6:31 AM.

Details

Summary

D88631 introduced a set of knobs to tweak how the stack protector is codegen'd for x86 targets, including the offset from the base register where the stack cookie is located. The StackProtectorGuardOffset field in TargetOptions was left uninitialized instead of being reset to its neutral value -1, making it possible to emit nonsensical code if the frontend doesn't change the field value at all before feeding the TargetOptions to the target machine initializer.

Diff Detail

Event Timeline

LemonBoy created this revision.Apr 6 2021, 6:31 AM
LemonBoy requested review of this revision.Apr 6 2021, 6:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 6 2021, 6:31 AM
llvm/include/llvm/Target/TargetOptions.h
334

Is the cast necessary?

LemonBoy added inline comments.Apr 6 2021, 10:58 AM
llvm/include/llvm/Target/TargetOptions.h
334

Not really, unless -Wsign-conversion or equivalent options are passed.
I'm partial to keeping the explicit cast, if there are strong reasons to remove it I'll get rid of it.

craig.topper added inline comments.Apr 6 2021, 10:59 AM
llvm/include/llvm/Target/TargetOptions.h
334

Use -1U?

LemonBoy updated this revision to Diff 335598.Apr 6 2021, 11:06 AM

Use suffix instead of an explicit cast.

LemonBoy marked 2 inline comments as done.Apr 6 2021, 11:07 AM
LemonBoy added inline comments.
llvm/include/llvm/Target/TargetOptions.h
334

Good call, thanks.

nickdesaulniers accepted this revision.Apr 6 2021, 11:32 AM
This revision is now accepted and ready to land.Apr 6 2021, 11:32 AM
xiangzhangllvm added inline comments.Apr 6 2021, 4:50 PM
llvm/include/llvm/Target/TargetOptions.h
334

Does it duplicated with
clang/include/clang/Basic/CodeGenOptions.def:369:VALUE_CODEGENOPT(StackProtectorGuardOffset, 32, (unsigned)-1) ?