Page MenuHomePhabricator

[clang-cl] Make /Gs imply default stack probes, not /Gs0 (PR39074)
ClosedPublic

Authored by hans on Sep 25 2018, 7:56 AM.

Diff Detail

Repository
rC Clang

Event Timeline

hans created this revision.Sep 25 2018, 7:56 AM
majnemer accepted this revision.Sep 25 2018, 11:19 AM
majnemer added a subscriber: majnemer.

FWIW, Microsoft's newest documentation does not say that /O2 and /O1 imply /Gs: https://docs.microsoft.com/en-us/cpp/build/reference/o1-o2-minimize-size-maximize-speed?view=vs-2017

This revision is now accepted and ready to land.Sep 25 2018, 11:19 AM
hans added a comment.Sep 26 2018, 12:34 AM

FWIW, Microsoft's newest documentation does not say that /O2 and /O1 imply /Gs: https://docs.microsoft.com/en-us/cpp/build/reference/o1-o2-minimize-size-maximize-speed?view=vs-2017

Great, thanks for finding that!

This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.

Thanks! lgtm.

include/clang/Driver/CLCompatOptions.td
94

https://docs.microsoft.com/en-us/cpp/build/reference/gs-control-stack-checking-calls?view=vs-2017 still claims that /Gs is /Gs0 though. Is that just wrong? https://bugs.llvm.org/show_bug.cgi?id=39074#c2 suggests it is. Should we ask bruce to file a doc bug?

And since this flag is supposed to get the default behavior, should it be a CLIgnoredFlag instead of duplicating the 4096 somewhat redundantly?

hans added inline comments.Sep 26 2018, 5:31 AM
include/clang/Driver/CLCompatOptions.td
94

I've submitted feedback on the document page: https://github.com/MicrosoftDocs/cpp-docs/issues/445

By not ignoring it, we enable using /Gs to override a previous e.g. /Gs0 flag, which I think is the correct thing to do.

Sounds great, thanks!