Page MenuHomePhabricator

[mips] Make the MipsAsmParser capable of knowing whether PIC mode is enabled or not.
ClosedPublic

Authored by tomatabacu on Oct 6 2014, 9:08 AM.

Details

Summary

This information is needed to decide whether we do the PIC-only JAL expansions or not. It's also needed for an upcoming patch which implements the .cprestore assembler directive (which can only be used effectively in PIC mode).

By making this information available to the MipsAsmParser, we will know when to insert the instructions mandated by the .cprestore assembler directive and we will be able to give some useful warnings when we encounter a potential misuse of this directive.

Diff Detail

Repository
rL LLVM

Event Timeline

tomatabacu updated this revision to Diff 14457.Oct 6 2014, 9:08 AM
tomatabacu retitled this revision from to [mips] Make the MipsAsmParser capable of knowing whether PIC mode is enabled or not..
tomatabacu updated this object.
tomatabacu edited the test plan for this revision. (Show Details)
tomatabacu added a reviewer: dsanders.
tomatabacu added a subscriber: Unknown Object (MLST).
dsanders edited edge metadata.Oct 7 2014, 2:09 AM

I'm not sure Reloc::PIC_ is the right thing to test for since '.option pic0' can be used to temporarily disable PIC in a PIC object. I think that MipsAsmParser needs to keep track of the PIC mode itself and have '.option pic0' and '.option pic2' change it appropriately.

tomatabacu updated this revision to Diff 14642.Oct 9 2014, 3:16 AM
tomatabacu edited edge metadata.

Addressed dsanders' concerns by using the PIC mode value set by '.option pic0/pic2', if these 2 directives are used.

tomatabacu updated this revision to Diff 15179.Oct 21 2014, 3:17 AM

Remove excess whitespace.

tomatabacu updated this revision to Diff 15180.Oct 21 2014, 3:27 AM

Did the diff from a clean branch instead of a development branch.

tomatabacu updated this revision to Diff 15870.Nov 6 2014, 7:24 AM

Renamed variable to have a clearer name.

dsanders added a subscriber: rafael.

This also needs a testcase. I understand there is a follow-up patch that effectively tests this. Could you link to it?

@rafael: We've noticed something odd in the MC class hierarchy for all targets. The details are below, could you take a look?

lib/Target/Mips/AsmParser/MipsAsmParser.cpp
105–106 ↗(On Diff #15870)

Why not initialize IsOptionDirPicEnabled from 'getContext().getObjectFileInfo()->getRelocM() == Reloc::PIC_' and drop UsePicFromOptionDir?

It turns out that doing this causes crashes and while looking into it again we've noticed something strange. It seems MipsAsmParser::Parser shadows MCAsmParserExtension::Parser. As a result, calling getContext() (which returns getParser().getContext()) may crash when getParser().getContext() is fine. The difference seems to be that getContext() uses MCAsmParserExtension::getParser() and MCAsmParserExtension::Parser whereas getParser().getContext() uses MipsAsmParser::getParser() and MipsAsmParser::Parser. MipsAsmParser::Parser is initialized by the constructor but (I think) MCAsmParserExtension is not.

@rafael: All the targets seem to do the same thing. Was it intentional?

rafael edited edge metadata.Nov 10 2014, 9:10 PM

It turns out that doing this causes crashes and while looking into it again we've noticed something strange. It seems MipsAsmParser::Parser shadows MCAsmParserExtension::Parser. As a result, calling getContext() (which returns getParser().getContext()) may crash when getParser().getContext() is fine. The difference seems to be that getContext() uses MCAsmParserExtension::getParser() and MCAsmParserExtension::Parser whereas getParser().getContext() uses MipsAsmParser::getParser() and MipsAsmParser::Parser. MipsAsmParser::Parser is initialized by the constructor but (I think) MCAsmParserExtension is not.

@rafael: All the targets seem to do the same thing. Was it intentional?

It was a bug. Fixed in r221667.

Cheers,
Rafael

tomatabacu updated this object.Nov 12 2014, 8:34 AM
tomatabacu removed a reviewer: rafael.
tomatabacu updated this revision to Diff 16098.Nov 12 2014, 8:44 AM

-we now track the status of PIC mode with a single bool member variable.
-made comments clearer.

The inPicMode() function is used in "[mips] Expand JAL instructions when PIC is enabled." (http://reviews.llvm.org/D6231).

dsanders accepted this revision.Nov 13 2014, 9:09 AM
dsanders edited edge metadata.

LGTM but please wait until "[mips] Expand JAL instructions when PIC is enabled." (http://reviews.llvm.org/D6231) is reviewed before you commit.

This revision is now accepted and ready to land.Nov 13 2014, 9:09 AM
brooks added a subscriber: brooks.Jul 21 2015, 2:15 PM
seanbruno accepted this revision.Aug 16 2015, 4:29 PM
seanbruno added a reviewer: seanbruno.

Thanks for the work on this.

dsanders closed this revision.Aug 18 2015, 5:34 AM
This revision was automatically updated to reflect the committed changes.