[ELF][PPC64] Add --lax-call-lacks-nop

[merge_guards_bot]

Unit tests: pass. 61237 tests passed, 0 failed and 729 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[MaskRay]

Add a hint. Add a test (we have to work around a MC deficiency)

[MaskRay]

Forgot to add the test

[merge_guards_bot]

Unit tests: pass. 61237 tests passed, 0 failed and 729 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[merge_guards_bot]

Unit tests: pass. 61238 tests passed, 0 failed and 729 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[ruiu]

lld/ELF/InputSection.cpp

980–984

Can this condition be reduced to

!config->laxCallLacksNop && rel.sym->file != file &&
(bufLoc + 8 > bufEnd || read32(bufLoc + 4) != 0x60000000)

?

lld/ELF/Options.td

225

Does this mean "GCC 5.4 or earlier AND GCC 6.4 or earlier"? Did you mean GCC 5.4 to 6.4?

[MaskRay]

Update help message

[merge_guards_bot]

Unit tests: pass. 61251 tests passed, 0 failed and 736 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[ruiu]

lld/ELF/Options.td

225

I"m confused. Doesn't "GCC 5.4 or earlier AND GCC 6.4 or earlier" just mean "GCC 5.4 or earlier"?

[MaskRay]

lld/ELF/InputSection.cpp

980–984

It can't.

rel.sym->file == file is the case we want to ignore when --lax-call-lacks-nop is specified.

So we use !(config->laxCallLacksNop && rel.sym->file == file) here.

lld/ELF/Options.td

225

Each major version has a release branch. This was fixed in 7 (not released at that time), then back ported to the release branches of 5 and 6. So certain 5.* and 6.* releases do not include the patch.

[ruiu]

lld/ELF/InputSection.cpp

980–984

OK. Can you factor out config->laxCallLacksNop && rel.sym->file == file as a boolean variable?

lld/ELF/Options.td

225

How about something like "GCC 5.0-5.6 and GCC 6.0-6.4"?

[MaskRay]

Update help message

[merge_guards_bot]

Unit tests: fail. 61245 tests passed, 6 failed and 736 were skipped.

failed: LLVM-Unit.ADT/_/ADTTests/APFloatTest.DecimalStringsWithoutNullTerminators
failed: Clang Tools.clang-tidy/checkers/readability-magic-numbers-bitfields.cpp
failed: Clang Tools.clang-tidy/checkers/readability-magic-numbers.cpp
failed: Clang Tools.clang-tidy/readability-magic-numbers-userliteral.cpp
failed: libc++.std/thread/thread_mutex/thread_mutex_requirements/thread_mutex_requirements_mutex/thread_mutex_recursive/lock.pass.cpp
failed: libc++.std/thread/thread_mutex/thread_mutex_requirements/thread_mutex_requirements_mutex/thread_mutex_recursive/try_lock.pass.cpp

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[sfertile]

Thanks for doing this. I think this approach in much safer at the cost of some friction on the systems where the option is needed. My one suggestion would be to separate the tail call case b foo from the normal call case. It will cause a runtime failure if the callers caller resides outside the dso and would be much more likely then a failure like glibc encountered. I believe gcc/gfortran got that case correct so it shouldn't impact usability on the older OS releases, but I'm not 100% sure of that.

lld/ELF/Config.h

struct Configuration {
   bool checkSections;
   bool compressDebugSections;
   bool cref;
   bool defineCommon;
   bool demangle = true;
   bool dependentLibraries;
   bool disableVerify;
   bool ehFrameHdr;
   bool emitLLVM;
   bool emitRelocs;
   bool enableNewDtags;
+  bool errorCallLacksNop;
   bool executeOnly;
   bool exportDynamic;
   bool fixCortexA53Errata843419;
   bool fixCortexA8;
   bool forceBTI;
   bool formatBinary = false;
   bool requireCET;
   bool gcSections;
   bool gdbIndex;
   bool gnuHash = false;
   bool gnuUnique;
   bool hasDynamicList = false;
   bool hasDynSymTab;
   bool ignoreDataAddressEquality;
   bool ignoreFunctionAddressEquality;
+  bool laxCallLacksNop;
   bool ltoCSProfileGenerate;
   bool ltoDebugPassManager;
   bool ltoNewPassManager;
   bool mergeArmExidx;
   bool mipsN32Abi = false;
   bool mmapOutputFile;
   bool nmagic;
   bool noinhibitExec;
   bool nostdlib;
   bool oFormatBinary;
   bool omagic;

lld/ELF/Driver.cpp

static void readConfigs(opt::InputArgList &args) {
   config->gnuUnique = args.hasFlag(OPT_gnu_unique, OPT_no_gnu_unique, true);
   config->gdbIndex = args.hasFlag(OPT_gdb_index, OPT_no_gdb_index, false);
   config->icf = getICF(args);
   config->ignoreDataAddressEquality =
       args.hasArg(OPT_ignore_data_address_equality);
   config->ignoreFunctionAddressEquality =
       args.hasArg(OPT_ignore_function_address_equality);
   config->init = args.getLastArgValue(OPT_init, "_init");
+  config->laxCallLacksNop = args.hasArg(OPT_lax_call_lacks_nop);
   config->ltoAAPipeline = args.getLastArgValue(OPT_lto_aa_pipeline);
   config->ltoCSProfileGenerate = args.hasArg(OPT_lto_cs_profile_generate);
   config->ltoCSProfileFile = args.getLastArgValue(OPT_lto_cs_profile_file);
   config->ltoDebugPassManager = args.hasArg(OPT_lto_debug_pass_manager);
   config->ltoNewPassManager = args.hasArg(OPT_lto_new_pass_manager);
   config->ltoNewPmPasses = args.getLastArgValue(OPT_lto_newpm_passes);
   config->ltoo = args::getInteger(args, OPT_lto_O, 2);
   config->ltoObjPath = args.getLastArgValue(OPT_lto_obj_path_eq);

lld/ELF/InputSection.cpp

case R_PPC64_CALL:
       if (read32(bufLoc) == 0x60000000) // nop
         break;
 
       // Patch a nop (0x60000000) to a ld.
       if (rel.sym->needsTocRestore) {
         // gcc/gfortran 5.4, 6.3 and earlier versions do not add nop for
         // recursive calls even if the function is preemptible. This is not
         // wrong in the common case where the function is not preempted at
-        // runtime. Just ignore.
+        // runtime. Just ignore if --lax-call-lacks-nop is specified.
+        bool ignore = config->laxCallLacksNop && rel.sym->file == file;
         if ((bufLoc + 8 > bufEnd || read32(bufLoc + 4) != 0x60000000) &&
-            rel.sym->file != file) {
+            !ignore) {
+          std::string hint;
+          if (rel.sym->file == file)
+            hint = "; pass -Wl,--lax-call-lacks-nop to ignore the error";
           // Use substr(6) to remove the "__plt_" prefix.
           errorOrWarn(getErrorLocation(bufLoc) + "call to " +
                       lld::toString(*rel.sym).substr(6) +
-                      " lacks nop, can't restore toc");
+                      " lacks nop, can't restore toc" + hint);
           break;
         }
         write32(bufLoc + 4, 0xe8410018); // ld %r2, 24(%r1)
       }
       target->relocateOne(bufLoc, type, targetVA);
       break;
     default:
       target->relocateOne(bufLoc, type, targetVA);

lld/ELF/Options.td

 
 defm init: Eq<"init", "Specify an initializer function">,
   MetaVarName<"<symbol>">;
 
 defm just_symbols: Eq<"just-symbols", "Just link symbols">;
 
 defm keep_unique: Eq<"keep-unique", "Do not fold this symbol during ICF">;
 
+def lax_call_lacks_nop : F<"lax-call-lacks-nop">,
+    HelpText<"(PowerPC64) Don't error on b/bl not followed by nop, if the target "
+             "is defined in the same file. This is for compatibility with "
+             "GCC<5, GCC 5.0~5.4, and GCC 6.0~6.3">;
+
 defm library: Eq<"library", "Root name of library to use">,
   MetaVarName<"<libName>">;
 
 def m: JoinedOrSeparate<["-"], "m">, HelpText<"Set target emulation">;
 
 defm Map: Eq<"Map", "Print a link map to the specified file">;
 
 defm merge_exidx_entries: B<"merge-exidx-entries",

lld/test/ELF/ppc64-error-toc-tail-call.s

 // REQUIRES: ppc
 
 // RUN: llvm-mc -filetype=obj -triple=powerpc64le-unknown-linux %s -o %t.o
 // RUN: llvm-mc -filetype=obj -triple=powerpc64le-unknown-linux %p/Inputs/shared-ppc64.s -o %t2.o
 // RUN: ld.lld -shared %t2.o -o %t2.so
 // RUN: not ld.lld %t.o %t2.so -o %t 2>&1 | FileCheck %s
 
 // RUN: llvm-mc -filetype=obj -triple=powerpc64-unknown-linux %s -o %t.o
 // RUN: llvm-mc -filetype=obj -triple=powerpc64-unknown-linux %p/Inputs/shared-ppc64.s -o %t2.o
 // RUN: ld.lld -shared %t2.o -o %t2.so
 // RUN: not ld.lld %t.o %t2.so -o %t 2>&1 | FileCheck %s
 
 # A tail call to an external function without a nop should issue an error.
-// CHECK: call to foo lacks nop, can't restore toc
-// CHECK-NOT: lacks nop
+// CHECK: call to foo lacks nop, can't restore toc{{$}}
     .text
     .abiversion 2
 
 .global _start
 _start:
   b foo
 
   // gcc/gfortran 5.4, 6.3 and earlier versions do not add nop for recursive
   // calls.
   b _start
   b _start

lld/test/ELF/ppc64-lax-call-lacks-nop.s

+# REQUIRES: ppc
+## gcc/gfortran 5.4, 6.3 and earlier versions do not add nop for recursive
+## calls. Test that --lax-call-lacks-nop can suppress the error.
+
+# RUN: llvm-mc -filetype=obj -triple=powerpc64le %s -o %t.o
+# RUN: not ld.lld -shared %t.o -o /dev/null 2>&1 | FileCheck --check-prefix=ERROR %s
+# RUN: ld.lld -shared --lax-call-lacks-nop %t.o -o /dev/null 2>&1 | count 0
+
+# ERROR: call to bar lacks nop, can't restore toc; pass -Wl,--lax-call-lacks-nop to ignore the error
+
+.global _start
+_start:
+  ## Unfortunately, MC does not emit a relocation if the target is
+  ## defined in the same section, so the following test case does not exercise
+  ## the code path.
+  ## b _start
+  ## b _start
+  ##
+  ## Define bar in another section to exercise the code path.
+  bl bar
+  bl bar
+
+.section .text.bar,"ax",@progbits
+.global bar
+bar:
+  blr