Omit PT_NOTE for SHT_NOTE without SHF_ALLOC

[emaste]

Summary

A non-alloc note section should not have a PT_NOTE program header.

Found while linking ghc (Haskell compiler) with lld on FreeBSD. Haskell emits a .debug-ghc-link-info note section (as the name suggests, it contains link info) as a SHT_NOTE section without SHF_ALLOC set.

For this case ld.bfd does not emit a PT_NOTE segment for .debug-ghc-link-info. lld previously emitted a PT_NOTE with p_vaddr = 0 and FreeBSD's rtld segfaulted when trying to parse a note at address 0.

llvm.org/pr37361
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=226872

[emaste]

@arichardson @dim I'm also looking for approval to commit the change to FreeBSD (perhaps in advance of committing upstream), with a bump of the FreeBSD-specific version to LLD_REVISION_STRING "326565-1200002" - I might as well solicit that via this review.

[dim]

I'm fine with this, and also with committing it into FreeBSD's copy of lld, but I'd like to see the opinion of @ruiu and/or @grimar too. (I take it @espindola is no longer being monitored...)

[emaste]

Yeah, @espindola was added by a Herald rule.

[emaste]

Committed to FreeBSD in r333401

[ruiu]

I have a few questions:

1. what is the content of .debug-ghc-link-info? If the section contains an optional, human-readable string, they might have just used .comment instead.
2. Does this patch work when we have a .debug-ghc-link-info and we also need to create a PT_NOTE section for other .note section?

[emaste]

1. what is the content of .debug-ghc-link-info? If the section contains an optional, human-readable string, they might have just used .comment instead.

I think it's the link invocation command line. It is human-readable, and I agree they could probably use .comment, but this is what they're producing today. I think they used this so strip will strip it as a .debug section.

2. Does this patch work when we have a .debug-ghc-link-info and we also need to create a PT_NOTE section for other .note section?

It does, this is actually the case in FreeBSD - we had one PT_NOTE containing our ABI tag and one containing the .debug-ghc-link-info content; with the patch we have just the ABI tag PT_NOTE. I think the test could be extended to confirm but I guess it's probably not necessary.

[ruiu]

2. Does this patch work when we have a .debug-ghc-link-info and we also need to create a PT_NOTE section for other .note section?

It does, this is actually the case in FreeBSD - we had one PT_NOTE containing our ABI tag and one containing the .debug-ghc-link-info content; with the patch we have just the ABI tag PT_NOTE. I think the test could be extended to confirm but I guess it's probably not necessary.

I think that the test would be useful because it covers the most common use case. Other than that, this change looks good.

[ruiu]

LGTM

[grimar]

This LGTM too.
I would add/change the test cases for notes though. Will post a patch for review.

lld/trunk/ELF/Writer.cpp

template <class ELFT> std::vector<PhdrEntry *> Writer<ELFT>::createPhdrs() {
   // mmap(2) with PROT_WRITE | PROT_EXEC, which is prohibited by default on
   // OpenBSD.
   if (Config->ZWxneeded)
     AddHdr(PT_OPENBSD_WXNEEDED, PF_X);
 
   // Create one PT_NOTE per a group of contiguous .note sections.
   PhdrEntry *Note = nullptr;
   for (OutputSection *Sec : OutputSections) {
-    if (Sec->Type == SHT_NOTE) {
+    if (Sec->Type == SHT_NOTE && (Sec->Flags & SHF_ALLOC)) {
       if (!Note || Sec->LMAExpr)
         Note = AddHdr(PT_NOTE, PF_R);
       Note->add(Sec);
     } else {
       Note = nullptr;
     }
   }
   return Ret;

lld/trunk/test/ELF/note-noalloc.s

+// REQUIRES: x86
+// RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-freebsd %s -o %t.o
+// RUN: ld.lld %t.o -o %t -shared
+// RUN: llvm-readobj -program-headers -sections %t | FileCheck %s
+
+// PR37361: A note without SHF_ALLOC should not create a PT_NOTE program
+// header (but should have a SHT_NOTE section).
+
+// CHECK: Name: .note.tag
+// CHECK: Type: SHT_NOTE
+// CHECK: Name: .debug.ghc-link-info
+// CHECK: Type: SHT_NOTE
+// CHECK-NOT: Type: SHT_NOTE
+
+// CHECK: Type: PT_NOTE
+// CHECK-NOT: Type: PT_NOTE
+
+        .section        .note.tag,"a",@note
+        .quad 1234
+
+        .section        .debug.ghc-link-info,"",@note
+        .quad 5678