Formatted.

Refactored scanEhFrameSection.

Update the patch to incorporate the latest changes in the master branch.

• Substituted a sorted vector for a DenseMap.
• Improved (hopefully) comments.

LGTM.

Use fetchIfLazy() instead of addEntrySymbol().

It looks like Phabricator doesn't let me download binary files. But overall the patch looks good.

Did you mean something like this?
I am inclined not to mix an entry symbol into Config->Undefined in order to preserve clarity.

Yes, this variant is more straightforward. However, as addUndefined() implicitly marks the symbol to be preserved by LTO, we now have to set that flag directly; overwise, many tests in the lto test-suite fail. Moreover, without that flag, the entry field in the ELF header is not filled correctly in case of LTO because symbol _start is missing.

Yes, if it's possible, I'd like to see all the patches (one way or another), just to have the whole picture.

We came across the issue in our environment, where an ELF linker is used only as an intermediate step and the value of entry field in the header is ignored. But in one of the libraries, we have a weak reference to _start, because sometimes this symbol might be defined for a special purpose. I can guess that for some other embedded environments and third-party libraries the same situation might take place.

The patch doesn't apply to the current tip clearly. And after applying it manually and fix compilation errors, I have two failing tests for llvm-cov: tools/llvm-cov/showLineExecutionCounts.cpp and tools/llvm-cov/threads.c, both of which fail with "Coverage segments not unique or sorted" assertion.

LGTM

I'd like to say that I'm quite happy with this set of patches. We use them for Arm32 target with relatively large executables, and they seem to work well. It'd be great if these patches are ported to the release branch as well.

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.