This is an archive of the discontinued LLVM Phabricator instance.

[mips] Improve handling of -fno-[pic/PIC] option
ClosedPublic

Authored by abeserminji on Mar 20 2018, 8:44 AM.

Details

Summary
  • On N64 ABI, -mno-abicalls is needed to disable PIC. Warning is reported when only -fno-pic/-fno-PIC is used. This is how GCC behaves.
  • An error is reported when -fno-pic or -fno-PIC is used in combination with -mabicalls.

Depends on D44381.

Diff Detail

Repository
rL LLVM

Event Timeline

abeserminji created this revision.Mar 20 2018, 8:44 AM
abeserminji changed the repository for this revision from rL LLVM to rC Clang.Mar 21 2018, 9:15 AM
abeserminji removed a subscriber: llvm-commits.
sdardis requested changes to this revision.Apr 19 2018, 3:39 AM

A quick comment on the error message, inlined. It's about the quality of the diagnostics.

include/clang/Basic/DiagnosticDriverKinds.td
336–337 ↗(On Diff #139118)

Can you fine tune this error message to say:

"ignoring '-fno-pic' option as it cannot be used with explicit use of -mabicalls and the N64 ABI" when -mabicalls is used on the commandline and:

"ignoring '-fno-pic' option as it cannot be used with implicit use of -mabicalls and the N64 ABI" when -mno-abicalls and -mabicalls are not present.

You should also report the precise pic/PIC/pie/PIE option used in the error message. You should be able to get it from the argument directly: http://llvm.org/doxygen/classllvm_1_1opt_1_1Arg.html

This revision now requires changes to proceed.Apr 19 2018, 3:39 AM

Comments resolved.

abeserminji marked an inline comment as done.Apr 26 2018, 3:15 AM
sdardis added inline comments.Apr 26 2018, 6:47 AM
include/clang/Basic/DiagnosticDriverKinds.td
340 ↗(On Diff #144092)

Use the %select{optionA|optionB|..|optionZ}$NUM operator here like:

"cannot be used with the %select{explicit|implicit}1 usage of -mabicalls and the N64 ABI"

Which eliminates the need for two separate warnings.

lib/Driver/ToolChains/Arch/Mips.cpp
242–250 ↗(On Diff #144092)

Then all of this can be simplified.

abeserminji marked 2 inline comments as done.

Comments resolved.

include/clang/Basic/DiagnosticDriverKinds.td
340 ↗(On Diff #144092)

Thanks for the hint. I simplified it now.

sdardis accepted this revision.Apr 30 2018, 3:45 AM

LGTM, with a touch up of the error message to match the others we have regarding -mabicalls. Some other minor nits inlined.

include/clang/Basic/DiagnosticDriverKinds.td
340 ↗(On Diff #144092)

Just saw the error messages above; so for style, you should reformat the message to be of the form "ignoring '%0' option as it cannt be used with %select{|implicit usage of}1-mabicalls and the N64 ABI".

This way all the error messages are consistent.

lib/Driver/ToolChains/CommonArgs.cpp
1024 ↗(On Diff #144344)

Nit: "When targeting the N64 ABI, PIC is the default, except in the case when the -mno-abicalls option is used."

test/Driver/mips-abicalls-warning.c
2 ↗(On Diff #144344)

Add some checks for -fno-pie, -fno-PIE.

This revision is now accepted and ready to land.Apr 30 2018, 3:45 AM
abeserminji marked 4 inline comments as done.

Comments resolved.

This revision was automatically updated to reflect the committed changes.