This is an archive of the discontinued LLVM Phabricator instance.

[X86] Extend some Linux special cases to cover kFreeBSD.
ClosedPublic

Authored by koriakin on Apr 14 2016, 4:01 AM.
Tokens
"Like" token, awarded by mohamed.maalej.

Details

Summary

Both Linux and kFreeBSD use glibc, so follow similiar code paths.
Add isTargetGlibc to check for this, and use it instead of isTargetLinux
in a few places.

Fixes PR22248 for kFreeBSD.

Diff Detail

Repository
rL LLVM

Event Timeline

koriakin retitled this revision from to [X86] Extend some Linux special cases to cover kFreeBSD..
koriakin updated this object.
koriakin set the repository for this revision to rL LLVM.
koriakin added a subscriber: llvm-commits.

Updated due to a conflict with r266806.

Please can you recreate as a full diff to get more context?

http://llvm.org/docs/Phabricator.html explains how you normally should aim to create your diff with something like

svn diff --diff-cmd=diff -x -U999999

Please can you recreate as a full diff to get more context?

http://llvm.org/docs/Phabricator.html explains how you normally should aim to create your diff with something like

svn diff --diff-cmd=diff -x -U999999

Uh? diff #2 already has full context, do you want to see full context for the obsolete diff too (I messed up on that one)?

Uh? diff #2 already has full context, do you want to see full context for the obsolete diff too (I messed up on that one)?

My mistake - was looking at an old tab somehow.

RKSimon added inline comments.May 2 2016, 6:00 AM
lib/Target/X86/X86ISelLowering.cpp
2004

Add comments detailing the glibc requirement? A reference if possible similar to the diff anove in X86ISelDAGToDAG.cpp.

2017

Add comments detailing the glibc requirement?

lib/Target/X86/X86Subtarget.cpp
234

Your comment lists the OS types and then you use the GLibc helper - would it be clearer to add a isTargetkFreeBSD() to the if() instead?

An alternative would be to update the comment to make it clear that glibc x86 32-bit targets always have 16-byte alignment, but I'm not sure if that is actually true? If it is, please add a reference.

Additionally, this needs to be tested - I don't think the stack protector tests cover this.

koriakin planned changes to this revision.May 2 2016, 7:06 AM
RKSimon accepted this revision.May 5 2016, 3:34 AM
RKSimon edited edge metadata.

LGTM

This revision is now accepted and ready to land.May 5 2016, 3:34 AM
This revision was automatically updated to reflect the committed changes.