[ELF] - Do not try to create .eh_frame_hdr for relocatable output.

[grimar]

Summary

.eh_frame_hdr is a header constructed for .eh_frame sections.
We do not proccess .eh_frame when doing relocatable output,
so should not try to create .eh_frame_hdr too.
Previous behavior without this patch is segfault.

Fixes PR32118.

[jhenderson]

Personally, I think that there should be a warning or error about incompatible command-line options in this case. A user has explicitly requested two different features that cannot be used together. We should tell them that -r wins out over --eh-frame-hdr.

I think it would make sense to put the test inside the same test as the normal .eh_frame_hdr test. That way we know for certain that an .eh_frame_hdr would definitely be created if -r was not specified, and that we aren't getting a spurious pass.

[emaste]

A user has explicitly requested two different features that cannot be used together.

In this case the user explicitly requested -r, and the compiler driver implicitly added --eh-frame-hdr. A sample command line that leads to this error:

cc -target x86_64-unknown-freebsd12.0 --sysroot=/tank/emaste/obj/tank/emaste/src/freebsd-xlld/tmp -B/tank/emaste/obj/tank/emaste/src/freebsd-xlld/tmp/usr/bin -O2 -pipe -std=gnu99 -Qunused-arguments -nostdlib -Wl,-dc -r -o cat.lo cat_stub.o /tank/emaste/obj/tank/emaste/src/freebsd-xlld/rescue/rescue//tank/emaste/src/freebsd-xlld/bin/cat/cat.o

It may well be the case that we should (also) change Clang to not add --eh-frame-hdr on relocatable links.

[emaste]

The FreeBSD components that prompted PR32118 link with this change.

Unable to test full image as lld now fails an assertion later in the build - PR to follow.

/tank/emaste/obj/tank/emaste/src/freebsd-xlld/tmp/usr/bin/ld: error: Section has different type from others with the same name autoload.o:(.eh_frame)
Assertion failed: (isa<X>(Val) && "cast<Ty>() argument of incompatible type!"), function cast, file ../include/llvm/Support/Casting.h, line 236.
cc: error: unable to execute command: Abort trap (core dumped)
cc: error: linker command failed due to signal (use -v to see invocation)

[joerg]

I'm not sure that clang should care at all about relocatable output, it is too much of a special case. IMO if you want to link relocatable output, you should call the linker directly.

[emaste]

IMO if you want to link relocatable output, you should call the linker directly.

That's probably true and something we should address in FreeBSD's build.

However, this is a regression from 4.0.0 so perhaps we should initially go with this D30566 patch in addition to a warning that --eh-frame-header is being ignored?

[grimar]

In D30566#691493, @jhenderson wrote:

I think it would make sense to put the test inside the same test as the normal .eh_frame_hdr test. That way we know for certain that an .eh_frame_hdr would definitely be created if -r was not specified, and that we aren't getting a spurious pass.

I probably would not do that. Normal .eh_frame_hdr test is a "eh-frame-hdr.s" for example, it is itself complete and large enough.
We already have 15 testcases named "relocatable-<something>.s" for testing relocatable output specific things. It is consistent for new test to go into that group.

[grimar]

• Added warning when -r used together with --eh-frame-hdr

[jhenderson]

In D30566#692868, @grimar wrote:

In D30566#691493, @jhenderson wrote:

I think it would make sense to put the test inside the same test as the normal .eh_frame_hdr test. That way we know for certain that an .eh_frame_hdr would definitely be created if -r was not specified, and that we aren't getting a spurious pass.

I probably would not do that. Normal .eh_frame_hdr test is a "eh-frame-hdr.s" for example, it is itself complete and large enough.
We already have 15 testcases named "relocatable-<something>.s" for testing relocatable output specific things. It is consistent for new test to go into that group.

Fair enough, since there's prior art for this.

Looks good from my point of view, but should probably get a LGTM from somebody with a little more familiarity with the code-base.

[ruiu]

ELF/Driver.cpp

230

Shouldn't we handle this as an error instead of a warning? All other incompatible combinations are handled as errors here.

[grimar]

ELF/Driver.cpp

230

According to comments compiler driver implicitly added --eh-frame-hdr, I afraid we probably may get many complains about link fails if this be a error().
Thats why I added it as a warning.

[ruiu]

ELF/Driver.cpp

230

I don't want to add a warning that is too verbose and not helpful for users. That kind of warnings would educate users to ignore warnings rather than fixing them. If compilers automatically adds --eh-frame-hdr to the command line, there's no way for users to fix it, so you need to ignore this error message all the time.

[grimar]

ELF/Driver.cpp

230

Users has a choice here - to call linker directly for generating relocatable output instead of calling compiler.

I have no strong opinion here though. I am ok to remove it.

[ruiu]

ELF/Driver.cpp

230

Does gcc add this option too?

[grimar]

ELF/Driver.cpp

230

Yes, gcc 5.4.1 do that.

[ruiu]

ELF/Driver.cpp

230

OK, then it is too pervasive to warn. Probably we should just ignore --eh-frame-hdr if -r is passed rather than annoy users with the warning message.

[grimar]

• Removed warning when --eh-frame-hdr and -r used together.

[ruiu]

LGTM

ELF/Driver.cpp

static void checkOptions(opt::InputArgList &Args) {
   // table which is a relatively new feature.
   if (Config->EMachine == EM_MIPS && Config->GnuHash)
     error("the .gnu.hash section is not compatible with the MIPS target.");
 
   if (Config->Pie && Config->Shared)
     error("-shared and -pie may not be used together");
 
   if (Config->Relocatable) {
+    if (Config->EhFrameHdr)
+      warn("--eh-frame-hdr is ignored when using -r");
+
     if (Config->Shared)
       error("-r and -shared may not be used together");
     if (Config->GcSections)
       error("-r and --gc-sections may not be used together");
     if (Config->ICF)
       error("-r and --icf may not be used together");
     if (Config->Pie)
       error("-r and -pie may not be used together");

ELF/Writer.cpp

In<ELFT>::RelaIplt = make<RelocationSection<ELFT>>(
       false /*Sort*/);
   Add(In<ELFT>::RelaIplt);
 
   In<ELFT>::Plt = make<PltSection<ELFT>>(Target->PltHeaderSize);
   Add(In<ELFT>::Plt);
   In<ELFT>::Iplt = make<PltSection<ELFT>>(0);
   Add(In<ELFT>::Iplt);
 
+  if (!Config->Relocatable) {
     if (Config->EhFrameHdr) {
       In<ELFT>::EhFrameHdr = make<EhFrameHeader<ELFT>>();
       Add(In<ELFT>::EhFrameHdr);
     }
-
-  if (!Config->Relocatable) {
     In<ELFT>::EhFrame = make<EhFrameSection<ELFT>>();
     Add(In<ELFT>::EhFrame);
   }
 
   if (In<ELFT>::SymTab)
     Add(In<ELFT>::SymTab);
   Add(In<ELFT>::ShStrTab);
   if (In<ELFT>::StrTab)

test/ELF/relocatable-eh-frame-hdr.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
+# RUN: ld.lld --eh-frame-hdr -r %t.o -o %t 2>&1 | FileCheck -check-prefix=WARN %s
+# RUN: llvm-readobj -s %t | FileCheck %s
+
+# WARN: --eh-frame-hdr is ignored when using -r
+
+# CHECK:       Sections [
+# CHECK-NOT:    Name: .eh_frame_hdr
+
+.section .foo,"ax",@progbits
+.cfi_startproc
+.cfi_endproc