[ELF] Fix incorrect deduplication of MergeSyntheticSection's

[andrewng]

Summary

The Data member of MergeSyntheticSection, which is used by ICF to check
equality of section contents, is always empty. This can result in
incorrect and dangerous deduplication when using --icf=all and
--ignore-data-address-equality.

This simple fix populates the Data of MergeSyntheticSection with the
intended contents in finalizeContents and uses Data for writeTo.

Fixes bug PR36910.

[• espindola]

Seems reasonable. I will just do a quick check on the performance impact.

[• espindola]

test/ELF/icf-not-equal-merged-sections.s

7

The test would be a bit more dependable if you checked the content of the output with objdump/readelf/readobj.

As written the test would still pass if we changed the icf message and regressed this fix.

[andrewng]

In D44898#1048263, @espindola wrote:

Seems reasonable. I will just do a quick check on the performance impact.

Yes, that would be my only concern too, but I decided to keep it simple first.

test/ELF/icf-not-equal-merged-sections.s

7

Good point! I will look at adding this.

[andrewng]

Updated test to address review comments.

[• espindola]

ELF/SyntheticSections.cpp

2442

At least clang is not finding an operator new[] to use. I had to manually call the Allocate function for this to build:

new (BAlloc.Allocate<uint8_t>(Size)) uint8_t[Size]

I guess we could also add new[] to Support/Allocator.h

[andrewng]

Updated to fix issue with clang compilation.

[• espindola]

LGTM.

Some tests got a bit faster (clang) and some a bit slower (scylla).

[ruiu]

Doesn't this increase lld's memory usage significantly? Mergeable string table tend to be large, and it seems with this patch you keep a copy of an entire section in memory.

[• espindola]

In D44898#1048634, @ruiu wrote:

Doesn't this increase lld's memory usage significantly? Mergeable string table tend to be large, and it seems with this patch you keep a copy of an entire section in memory.

True. That would be a reasonable explanation for scylla getting slower.

We could inspect the individual buffers in the StringTableBuilders, but it is not clear if it is worth it.

Maybe the best option is just disabling ICF of synthetic sections?

[ruiu]

How much did Scylla get slower?

Maybe the best option is just disabling ICF of synthetic sections?

That sounds like a better way of fixing the issue.

[andrewng]

In D44898#1048688, @ruiu wrote:

How much did Scylla get slower?

Maybe the best option is just disabling ICF of synthetic sections?

That sounds like a better way of fixing the issue.

Yes, for large mergeable sections, this could significantly increase memory usage.

Disabling ICF for synthetic sections would be a way to fix the issue and I don't think it would have much negative impact in practice, i.e. ICF of these sections is not common.

[• espindola]

In D44898#1048688, @ruiu wrote:

How much did Scylla get slower?

2.8%. Somehow clang-fsds got 2.39% faster.

page faults got worse on all test cases, but on scylla they got worse by 15%. That is consistent with the extra allocations being responsible for the slowdown.

[ruiu]

2.8%. Somehow clang-fsds got 2.39% faster.

That speedup is interesting. I don't have a good theory to explain it though.

Even if this patch is somewhat performance-neutral, we still should avoid extra memory allocation if we can. Let's try ignoring synthetic sections in ICF. That's I think better.

[andrewng]

In D44898#1048789, @ruiu wrote:

2.8%. Somehow clang-fsds got 2.39% faster.

That speedup is interesting. I don't have a good theory to explain it though.

Even if this patch is somewhat performance-neutral, we still should avoid extra memory allocation if we can. Let's try ignoring synthetic sections in ICF. That's I think better.

Alternative fix here: D44923.

[• espindola]

We can close this one, right?

[andrewng]

Abandoned in favour of D44923.