[PATCH] D16045: [Coverage]: Fixing bug: -fcoverage-mapping does not work with gc-sections

[Xinliang David Li via llvm-commits]

On Mon, Jan 11, 2016 at 11:24 AM, Justin Bogner <mail at justinbogner.com> wrote:
> David Li <davidxl at google.com> writes:
>> davidxl created this revision.
>> davidxl added reviewers: bogner, vsk.
>> davidxl added a subscriber: llvm-commits.
>> Herald added a subscriber: emaste.
>>
>> (The patch is intended for discussion purpose only)
>>
>> With linker gc turned on, the linker will happily discard all
>> __llvm_covmap sections from input objects so resulting executable file
>> won't be usable with llvm-cov tool -- missing coverage data.
>>
>> To demonstrate the problem, build a program with
>> -fprofile-instr-generate -fcoverage-mapping -Wl,--gc-sections. The
>> resulting exec won't be usable for coverage data dumping.
>
> This is only relevant for ELF, right? The darwin linker doesn't have a
> --gc-sections flag, and similar flags like -dead_strip don't seem to
> exhibit the problem you're discussing here.
>

Looks like so.

>> This patch shows one way to to fix the problem (demonstrated for
>> Linux/FreeBSD only here).  The idea is to force referencing coverage
>> data module headers in profile dumper routines.
>>
>> There are a couple of downsides:
>> 1) it works for linux/freeBSD, and can be extended for
>> Darwin. Supporting on other platforms can be more complicated -- need
>> runtime registration of coverage data start/end
>> 2) increases runtime overhead (physical memory and dumping time)
>>
>> Another much better possible solution (verified manually on Linux) is
>> quite simple -- set __llvm_covmap section with the right attributes
>> such that they are not allocatable.  When a section does not have the
>> 'a' bit in attribute, the linker won't garbage collect it (just like
>> debug sections).
>>
>> This method has another added benefit --- process virtual memory size
>> is reduced and the covmap section is now strippable.  I think we
>> should go with this approach instead (at least for ELF based
>> systems). Thoughts?
>
> Yes, this sounds better. Is there any downside?

Not I am aware of. I will prepare patch that does that.

thanks,

David

>
>> http://reviews.llvm.org/D16045
>>
>> Files:
>>   lib/profile/InstrProfiling.h
>>   lib/profile/InstrProfilingFile.c
>>   lib/profile/InstrProfilingPlatformLinux.c
>>
>>
>> Index: lib/profile/InstrProfilingPlatformLinux.c
>> ===================================================================
>> --- lib/profile/InstrProfilingPlatformLinux.c
>> +++ lib/profile/InstrProfilingPlatformLinux.c
>> @@ -19,6 +19,9 @@
>>  #define PROF_CNTS_START INSTR_PROF_SECT_START(INSTR_PROF_CNTS_SECT_NAME)
>>  #define PROF_CNTS_STOP INSTR_PROF_SECT_STOP(INSTR_PROF_CNTS_SECT_NAME)
>>
>> +#define COVMAP_START INSTR_PROF_SECT_START(INSTR_PROF_COVMAP_SECT_NAME)
>> +#define COVMAP_STOP INSTR_PROF_SECT_STOP(INSTR_PROF_COVMAP_SECT_NAME)
>> +
>>  /* Declare section start and stop symbols for various sections
>>   * generated by compiler instrumentation.
>>   */
>> @@ -28,6 +31,8 @@
>>  extern uint64_t PROF_CNTS_STOP COMPILER_RT_VISIBILITY;
>>  extern char PROF_NAME_START COMPILER_RT_VISIBILITY;
>>  extern char PROF_NAME_STOP COMPILER_RT_VISIBILITY;
>> +extern __llvm_covmap_header COVMAP_START COMPILER_RT_VISIBILITY;
>> +extern __llvm_covmap_header COVMAP_STOP COMPILER_RT_VISIBILITY;
>>
>>  /* Add dummy data to ensure the section is always created. */
>>  __llvm_profile_data
>> @@ -56,4 +61,15 @@
>>  COMPILER_RT_VISIBILITY uint64_t *__llvm_profile_end_counters(void) {
>>    return &PROF_CNTS_STOP;
>>  }
>> +
>> +COMPILER_RT_VISIBILITY const __llvm_covmap_header *
>> +__llvm_profile_begin_covmap(void) {
>> +  return &COVMAP_START;
>> +}
>> +
>> +COMPILER_RT_VISIBILITY const __llvm_covmap_header *
>> +__llvm_profile_end_covmap(void) {
>> +  return &COVMAP_STOP;
>> +}
>> +
>>  #endif
>> Index: lib/profile/InstrProfilingFile.c
>> ===================================================================
>> --- lib/profile/InstrProfilingFile.c
>> +++ lib/profile/InstrProfilingFile.c
>> @@ -205,6 +205,7 @@
>>
>>  COMPILER_RT_VISIBILITY
>>  int __llvm_profile_write_file(void) {
>> +  __llvm_covmap_header *CovHeader;
>>    int rc;
>>
>>    GetEnvHook = &getenv;
>> @@ -227,7 +228,19 @@
>>    rc = writeFileWithName(__llvm_profile_CurrentFilename);
>>    if (rc)
>>      PROF_ERR("LLVM Profile: Failed to write file \"%s\": %s\n",
>> -            __llvm_profile_CurrentFilename, strerror(errno));
>> +             __llvm_profile_CurrentFilename, strerror(errno));
>> +
>> +  /* Force reference to coverage data.  */
>> +  for (CovHeader = __llvm_profile_begin_covmap();
>> +       CovHeader < __llvm_profile_end_covmap();) {
>> +    uint32_t CoverageSize = CovHeader->CoverageSize;
>> +    uint32_t NR = CovHeader->NRecords;
>> +    uint32_t FilenamesSize = CovHeader->FilenamesSize;
>> +    CovHeader =
>> +        (__llvm_covmap_header *)((char *)CovHeader +
>> +                                 NR * sizeof(__llvm_covmap_func_record) +
>> +                                 CoverageSize + FilenamesSize);
>> +  }
>>    return rc;
>>  }
>>
>> Index: lib/profile/InstrProfiling.h
>> ===================================================================
>> --- lib/profile/InstrProfiling.h
>> +++ lib/profile/InstrProfiling.h
>> @@ -30,6 +30,16 @@
>>  #include "InstrProfData.inc"
>>  } __llvm_profile_header;
>>
>> +typedef struct __llvm_covmap_header {
>> +#define COVMAP_HEADER(Type, LLVMType, Name, Initializer) Type Name;
>> +#include "InstrProfData.inc"
>> +} __llvm_covmap_header;
>> +
>> +typedef struct __llvm_covmap_func_record {
>> +#define COVMAP_FUNC_RECORD(Type, LLVMType, Name, Initializer) Type Name;
>> +#include "InstrProfData.inc"
>> +} __llvm_covmap_func_record;
>> +
>>  /*!
>>   * \brief Get number of bytes necessary to pad the argument to eight
>>   * byte boundary.
>> @@ -55,6 +65,8 @@
>>  const char *__llvm_profile_end_names(void);
>>  uint64_t *__llvm_profile_begin_counters(void);
>>  uint64_t *__llvm_profile_end_counters(void);
>> +const __llvm_covmap_header *__llvm_profile_begin_covmap(void);
>> +const __llvm_covmap_header *__llvm_profile_end_covmap(void);
>>
>>  /*!
>>   * \brief Clear profile counters to zero.