[ELF] - Mention -fPIC in some error messages

[grimar]

Summary

This is PR32429.

We did not mention -fPIC in error about producing dynamic relocation
in readonly segment before. Patch changes that.

[peter.smith]

I think that this is a worthwhile addition, I've seen at least a couple of questions on llvm-dev that this improvement might have helped to resolve.

ELF/Relocations.cpp

570

Typo: permited should be permitted.

We may want a bit more context with the -z notext option as well as to someone unfamiliar with the options could be a compiler or a linker option, and it isn't clear from the name what it will actually do. May I suggest something like:
"Unintentional use of such relocations is undesirable, and is not permitted by default, use linker option -z notext to allow.", not a strong opinion though.

[ruiu]

test/ELF/dynamic-reloc-in-ro.s

5–10

This error message style is not desirable, as two list elements ("defined in ..." and "referenced by ..." and the description text "Usually ..." are in the same list. That is just like this

Found an error
 - at file foo.c
 - on line 100
 - Usually this error happens when you made a mistake blah blah

and I believe you can see why this is wrong.

I also wonder if you really want to mention -z notext. We want to recommend or endorse the option because we want to avoid text relocations in general. In most cases, you just need to compile object files with -fPIC, so I believe you want to mention only that.

Overall, I'd change the message to "can't create dynamic relocation R_X86_64_64 against local symbol in readonly segment; recompile object files with -fPIC to fix this error".

[ruiu]

I also wonder if you really want to mention -z notext. We want to recommend or endorse the option because we want to avoid text relocations in general. In most cases, you just need to compile object files with -fPIC, so I believe you want to mention only that.

Sorry, I mean "We do not want to recommend or ..."

[grimar]

test/ELF/dynamic-reloc-in-ro.s

5–10

I also wonder if you really want to mention -z notext. We want to recommend or endorse the option because we want to avoid text relocations >in general. In most cases, you just need to compile object files with -fPIC, so I believe you want to mention only that.

Reason behind mentioning -z notext is that it is by default in GNU linkers and LLD has -z text as default behavior.
It may we worth to mention as it not obvious and I expect users generally not know about this difference and
wondering why ld.bfd links fine but LLD errors out.

[ruiu]

test/ELF/dynamic-reloc-in-ro.s

5–10

But how many moderm programs need text relocations? Printing out a hint for a rare error cause is more confusing than helpful. Even if you want to print out that hint, you still don't want to write it in a such way that you are not recommending the -z notext flag.

[grimar]

• Addressed comments.

test/ELF/dynamic-reloc-in-ro.s

5–10

Well. After playing with different ways of printing this error message I think am inclined to agree with your first suggestion.
Updated the diff.

(FWIW my last attemp was:

std::string Hint =
    "\n>>> Unintentional use of such relocations is undesirable, and not "
    "permitted by default"
    "\n>>> because makes impossible to share DSO code between processes."
    "\n>>> Normally this error happens when -fPIC is missing during "
    "compilation."
    "\n>>> (-z notext option can be used to ignore this error)";

)

It is probably too large to report.

[ruiu]

LGTM

ELF/Relocations.cpp

static RelExpr adjustExpr(SymbolBody &Body, RelExpr Expr, uint32_t Type,
   bool IsWrite = !Config->ZText || (S.Flags & SHF_WRITE);
   if (IsWrite || isStaticLinkTimeConstant<ELFT>(Expr, Type, Body, S, RelOff))
     return Expr;
 
   // This relocation would require the dynamic linker to write a value to read
   // only memory. We can hack around it if we are producing an executable and
   // the refered symbol can be preemepted to refer to the executable.
   if (Config->Shared || (Config->Pic && !isRelExpr(Expr))) {
+    std::string Hint =
+        "\n>>> Usually this error happens when -fPIC flag is missing during "
+        "compilation."
+        "\n>>> Unintentional use of such relocations is undesirable, and not "
+        "\n>>> permited by default, consider using -z notext.";
     error("can't create dynamic relocation " + toString(Type) + " against " +
           (Body.getName().empty() ? "local symbol"
                                   : "symbol: " + toString(Body)) +
-          " in readonly segment" + getLocation<ELFT>(S, Body, RelOff));
+          " in readonly segment" + getLocation<ELFT>(S, Body, RelOff) + Hint);
     return Expr;
   }
 
   if (Body.getVisibility() != STV_DEFAULT) {
     error("cannot preempt symbol: " + toString(Body) +
           getLocation<ELFT>(S, Body, RelOff));
     return Expr;
   }

test/ELF/dynamic-reloc-in-ro.s

 // REQUIRES: x86
 // RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
 // RUN: not ld.lld %t.o -o %t.so -shared 2>&1 | FileCheck %s
 
-// CHECK: can't create dynamic relocation R_X86_64_64 against local symbol in readonly segment
-// CHECK: >>> defined in {{.*}}.o
-// CHECK: >>> referenced by {{.*}}.o:(.text+0x0)
-
+// CHECK:      can't create dynamic relocation R_X86_64_64 against local symbol in readonly segment
+// CHECK-NEXT: >>> defined in {{.*}}.o
+// CHECK-NEXT: >>> referenced by {{.*}}.o:(.text+0x0)
+// CHECK-NEXT: >>> Usually this error happens when -fPIC flag is missing during compilation.
+// CHECK-NEXT: >>> Unintentional use of such relocations is undesirable, and not
+// CHECK-NEXT: >>> permited by default, consider using -z notext.
 foo:
 .quad foo