This is a copy of a comment from D34689.

It is possible that a precreated thunk section stays empty in the first pass, but is used in the next pass. In that case, it isn't added into ISR in ThunkCreator::mergeThunks(). For example:

$ cat a.s
	.global _start, foo
.section .text.start,"ax",%progbits
_start:
	bl _start
.section .text.dummy1,"ax",%progbits
.space 0xfffffe
.section .text.foo,"ax",%progbits
foo:
	bl _start
$ llvm-mc -filetype=obj -triple=thumbv7a-none-linux-gnueabi a.s -o a.o
$ ld.lld a.o -o a.out
$ objdump -d a.out
a.out:     file format elf32-littlearm

As far as I could check, other sections are not affected by the similar problem. Most of them don't call relocate*() methods. InputSection::writeTo uses unshifted Buf when calling relocate(). And EhInputSection avoids the problem because SectionBase::getOffset() doesn't add OutSecOff for them.

Ping.

Done: rL286337.

The test is too implementation-specific, and trying to override a function like calloc makes it fragile. Unfortunately, I didn't find a better way to test the library's behavior in case of memory exhaustion, but now I believe that this test doesn't add any significant value. I'm going to remove it if no one objects.

Ping.

This test is extremely fragile. Maybe, it is better to disable it, or even delete it altogether.

Ping.

LGTM.

LGTM.

@rmaprath: There are some issues in the test which upset build bots and I need some time to fix them. You don't need to wait for me, I'll put my patch on the top of yours when my fix is ready.

The patch is under review for a long time and the described problem is still here. As we've seen the issue in practice, I'm sure that others may also run into it, and I do believe it should be fixed, one way or another.

Ping...

• Removed a default value in emitSourceRegions(), added the corresponding argument to its call in EmptyCoverageMappingBuilder::write().
• Removed an additional sorting step in CounterCoverageMappingBuilder::write(), the patch is now depends on the extended sorting creteria in CoverageMappingWriter::write(), see D24034.

This patch fixes the following issue:

$ cat > test.cpp << EOF
void dummy() {}

I figured out that the last issue is an independent one, so I prepared a separate fix for it: D23987.

The issue in the current implementation of the libc++abi library looks like a time bomb. Even though its probability could be considered as very low, in fact, it depends on the type of an application. At least, we ran into it in our environment.

They are really helpful, especially for looking into things like SmallVector, which uses AlignedCharArrayUnion to store items. Personally, I'd prefer debug dumpers for QtCreator, but this variant is more universal.

I've reduced the last sample to the following:

    0|    1|void dummy() {}
     |    2|
    0|    3|#define MACRO_1 dummy()
     |    4|
    1|    5|#define MACRO_2 MACRO_1
     |    6|
    1|    7|int main() {
    1|    8|  int i = 0;
    1|    9|  if (i > 5)
    1|   10|    MACRO_2;
------------------
|  |      1|    5|#define MACRO_2 MACRO_1
|  |  ------------------
|  |  |  |      0|    3|#define MACRO_1 dummy()
|  |  ------------------
------------------
    1|   11|  return 0;
    1|   12|}

Right now, I don't have a good idea how to fix this.

In D23160#506061, @vsk wrote:

I guess it never makes sense to have two regions with the exact same start/end loc, and different counters. Do you think we should add assertions in llvm (either in llvm-cov, or in the coverage reader) which guard against this?

The motivation sample:

$ cat > test.cpp << EOF
void dummy() {}

This patch is under review for more than two months, and I believe I've addressed all the comments. Could someone take a look at it, please?

Ping.

I agree that it'd be expensive to put a new counter after each function call. Even if there is no throw statement within a TU, called functions still may raise exceptions.
Anyway, if we come across a try statement, we can guess that some exceptions are expected and do our best to support constructions like this:

try {
  ...
}
catch (...) {
  ...
  throw;
}

• Use StartFileID and EndFileID variables to eliminate redundant calls to SM.getFileID().
• Add comment strings to asserts.

So, our tool isn't accurate in the case of throwing an exception. Is there a case where this patch makes things better than they were before? Is it possible to improve handling of exceptions instead?

This change is not needed anymore because the whole issue was fixed in D20286.

• Rebased to the top
• Extracted the code of filling the mapping records collection, including duplicate resolution logic, into a separate function.

• Added the class to check if the coverage mapping data is dummy or real.
• New implementations also works for simple functions with zero hash, so D20287 is not needed anymore.
• The test case was extended to check simple inline functions in addition to regular ones.

In D20287#431055, @davidxl wrote:

On the other hand, I wonder what is the real root cause of the problem. The dummy function record does not have its 'own' profile counts, so

if (std::error_code EC = ProfileReader.getFunctionCounts(
            Record.FunctionName, Record.FunctionHash, Counts)) {

call in CoverageMapping::load (..)

method should return the the counts of the used function even with zero functionhash. What did I miss?

Does anyone known, why we need dummy coverage mapping records for unused functions? How are they used? Isn't it better to remove these dummy records to prevent confusion with the real ones?

The motivation sample (using llvm-cov with D20286 applied):

$ cat > sample.h << EOF
inline int sample_func(int A) {
  return A;
}
EOF
$ cat > dummy.cpp << EOF
#include "sample.h"
EOF
$ cat > sample.cpp << EOF
#include "sample.h"

The motivation sample for this patch is the following:

$ cat > sample.h << EOF
inline int sample_func(int A) {
  if (A > 0)
    return A;
  return 0;
}
EOF
$ cat > dummy.cpp << EOF
#include "sample.h"
EOF
$ cat > sample.cpp << EOF
#include "sample.h"

Ping?..

Ping...