[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 ↗

(On Diff #90662)

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 ↗

(On Diff #90662)

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 ↗

(On Diff #90662)

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 ↗

(On Diff #90662)

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 ↗

(On Diff #90662)

Does gcc add this option too?

[grimar]

ELF/Driver.cpp

230 ↗

(On Diff #90662)

Yes, gcc 5.4.1 do that.

[ruiu]

ELF/Driver.cpp

230 ↗

(On Diff #90662)

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

lld/trunk/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)

lld/trunk/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
+# RUN: llvm-readobj -s %t | FileCheck %s
+
+# CHECK:       Sections [
+# CHECK-NOT:    Name: .eh_frame_hdr
+
+.section .foo,"ax",@progbits
+.cfi_startproc
+.cfi_endproc