[Coverage] Do not allocate memory for coverage map data (Linux)

[davidxl]

Summary

Coverage mapping data is not referenced by runtime, and they won't be dumped into profile data. There is no need to allocate memory for covmap sections.

A good side effect of this change is that the coverage map data won't be mistakenly garbage collected by the linker (for Gold linker only, BFD linker has an issue where the a bug is filed).

Tested with clang build with instrumentation and -fcoverage-mapping and linker GC. The size of covmap section is ~17.6M so the text segment size will be reduced by this amount with this change.

Need suggestion on how this is done for Mach-O.

[davidxl]

Discussed with bfd linker maintainer: making the section type SHT_NOTE, the GC won't happen for bfd linker either (which also works for gold).

A test case will be added in a separate patch (in compile_rt).

[davidxl]

http://reviews.llvm.org/D16118 is the test case depending on this patch.

[vsk]

Along with the compiler-rt test I think this looks fine, but caveat: I'm not a linker expert at all!

After some discussion with @pete and @lgerbarg, it looks like there isn't a short term way to achieve this with Mach-O. To recap:

1. Passing in -Wl,-dead_strip -fprofile-instr-generate -fcoverage-mapping doesn't get rid of __llvm_covmap because the linker is conservative about sections it doesn't 'know' about.
2. Because this section is in the __DATA segment, it will be mapped into the process along with the rest of the segment. We could pull this out into a separate segment (along with debug info?) and experiment.

[davidxl]

Ok -- I will leave Mach-O change out for now then.

(for experimental purpose, you can dot this quickly by modifying .s
file generated by the compiler).

thanks,

David

[vsk]

Good point, it's a simple change to rename the segment name in the .s file. However vmmap shows that the segment is still mapped into the process (as lgerbarg expects):

                                VIRTUAL   REGION 
REGION TYPE                        SIZE    COUNT (non-coalesced) 
===========                     =======  ======= 
Activity Tracing                  2048K        2 
Kernel Alloc Once                    4K        2 
MALLOC guard page                   16K        4 
...
STACK GUARD                       56.0M        2 
Stack                             8192K        2 
__COV                               36K        2

[davidxl]

This is with cov data stored in __DWARF segment?

David

[vsk]

I changed the .section directive to .section __COV,__llvm_covmap per your suggestion. Placing the covmap section in the __DWARF segment results in the same behavior (it is mapped into process memory).

[davidb]

Hi,

The section type has been changed to SHT_NOTE, however the section contents has not been changed to match the NOTE section format. This has caused issues with some of our tools and may affect others that use note sections. There is a description of the note section format here:

http://docs.oracle.com/cd/E19253-01/817-1984/chapter6-18048/index.html

Can't the section type remain as SHT_PROGBITS, but remove any section flags (SHF_ALLOC) that may cause the section to be allocated to a loadable segment?

[davidxl]

The problem with marking the section SFH_ALLOC only is that the BFD
linker can not handle it properly -- see
https://sourceware.org/bugzilla/show_bug.cgi?id=19446 for details.

As regarding the note section format, AFAIK, there is no formal
specification in ELF. The organization mentioned in the document is
for illustration purpose only.

thanks,

David

[davidb]

As regarding the note section format, AFAIK, there is no formal
specification in ELF. The organization mentioned in the document is
for illustration purpose only.

thanks,

David

I have found this description of note section in almost every ELF file format document I have, including the System V gABI. The GNU binutils such as readelf expect sections of SHT_NOTE to be in this format and will try to process it as such.

marking these sections in linker script with a KEEP directive sounds like a much more appropriate fix.

[vsk]

Done in llvm/r257781

[vsk]

Closing old review

lib/CodeGen/TargetLoweringObjectFileImpl.cpp

 #include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCExpr.h"
 #include "llvm/MC/MCSectionCOFF.h"
 #include "llvm/MC/MCSectionELF.h"
 #include "llvm/MC/MCSectionMachO.h"
 #include "llvm/MC/MCStreamer.h"
 #include "llvm/MC/MCSymbolELF.h"
 #include "llvm/MC/MCValue.h"
+#include "llvm/ProfileData/InstrProf.h"
 #include "llvm/Support/COFF.h"
 #include "llvm/Support/Dwarf.h"
 #include "llvm/Support/ELF.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Target/TargetLowering.h"
 #include "llvm/Target/TargetMachine.h"
 #include "llvm/Target/TargetSubtargetInfo.h"

 static SectionKind
 getELFKindForNamedSection(StringRef Name, SectionKind K) {
   // N.B.: The defaults used in here are no the same ones used in MC.
   // We follow gcc, MC follows gas. For example, given ".section .eh_frame",
   // both gas and MC will produce a section with no flags. Given
   // section(".eh_frame") gcc will produce:
   //
   //   .section   .eh_frame,"a",@progbits
+  
+  if (Name == getInstrProfCoverageSectionName(false))
+    return SectionKind::getMetadata();
+
   if (Name.empty() || Name[0] != '.') return K;
 
   // Some lame default implementation based on some magic section names.
   if (Name == ".bss" ||
       Name.startswith(".bss.") ||
       Name.startswith(".gnu.linkonce.b.") ||
       Name.startswith(".llvm.linkonce.b.") ||
       Name == ".sbss" ||
if (Name == ".tbss" ||
     return SectionKind::getThreadBSS();
 
   return K;
 }
 
 
 static unsigned getELFSectionType(StringRef Name, SectionKind K) {
 
+  if (Name == getInstrProfCoverageSectionName(false))
+    return ELF::SHT_NOTE;
+
   if (Name == ".init_array")
     return ELF::SHT_INIT_ARRAY;
 
   if (Name == ".fini_array")
     return ELF::SHT_FINI_ARRAY;
 
   if (Name == ".preinit_array")
     return ELF::SHT_PREINIT_ARRAY;