This is an archive of the discontinued LLVM Phabricator instance.

[X86] Add -mseparate-stack-seg
Needs ReviewPublic

Authored by mlemay-intel on Feb 10 2016, 1:20 PM.

Details

Summary

This patch adds the -mseparate-stack-seg option to enable restricting accesses to help prevent stack corruption.

Event Timeline

mlemay-intel retitled this revision from to [X86] Add -mseparate-stack-seg.
mlemay-intel updated this object.
delena added a subscriber: delena.Feb 22 2016, 11:12 PM
delena added inline comments.
lib/CodeGen/TargetInfo.cpp
1569 ↗(On Diff #47511)

Hi,
I'm not clang reviewer at all and you can ignore my comment.

Searching string looks strange for me. I suppose that this string should be defined in another form. Something like
options::OPT_separate_stack_seg.

1577 ↗(On Diff #47511)

You should not use 258 as a constant. It should be defined somewhere as address space enum.

1578 ↗(On Diff #47511)

This line alignment does not match LLVM style.

1580 ↗(On Diff #47511)

Again, not sure that I'm right. You are trying to create addressspacecast. Is it the right way to create ptrtoint + inttoptr?

Thank you for your feedback!

lib/CodeGen/TargetInfo.cpp
1569 ↗(On Diff #47511)

I haven't quite found an example of how an option like this should be handled. String comparisons of this form are performed in clang/lib/Basic/Targets.cpp, but there they are used to initialize variables. Furthermore, all of those tests are for CPU features, whereas this one is for a particular segment configuration.

1577 ↗(On Diff #47511)

I agree with this principle. However, the address space numbers for FS and GS are listed in the Clang documentation [1] and are used as literals in LLVM code. Thus, I think this patch is consistent with existing usage of address space numbers.

[1] http://clang.llvm.org/docs/LanguageExtensions.html#memory-references-off-the-gs-segment

1578 ↗(On Diff #47511)

I'll fix it.

1580 ↗(On Diff #47511)

I first attempted to create an address space cast, but LLVM disallows that. I think it is necessary to perform the conversion through an int, but I am not entirely sure of that.

qcolombet resigned from this revision.Mar 8 2016, 2:51 PM
qcolombet edited reviewers, added: craig.topper; removed: qcolombet.

Hi Craig,

Could you have a look? I am not confident enough with clang to give more feedback than Elena.

Cheers,
-Quentin

Revise patch to fix line alignment.

mkuper resigned from this revision.May 6 2016, 12:58 PM
mkuper removed a reviewer: mkuper.

I really don't know enough about this part of clang either.

Removed the portions that are specific to 32-bit segmentation. I plan to resubmit those later as a separate patch.

mlemay-intel edited the summary of this revision. (Show Details)Feb 7 2017, 8:28 AM