Page MenuHomePhabricator

[ELF] Fix incorrect deduplication of MergeSyntheticSection's
AbandonedPublic

Authored by andrewng on Mar 26 2018, 8:29 AM.

Details

Reviewers
ruiu
espindola
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.

Diff Detail

Event Timeline

andrewng created this revision.Mar 26 2018, 8:29 AM

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

espindola added inline comments.Mar 26 2018, 8:53 AM
test/ELF/icf-not-equal-merged-sections.s
6

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.

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
6

Good point! I will look at adding this.

andrewng updated this revision to Diff 139803.Mar 26 2018, 9:13 AM

Updated test to address review comments.

espindola added inline comments.Mar 26 2018, 9:19 AM
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 this revision to Diff 139808.Mar 26 2018, 9:38 AM

Updated to fix issue with clang compilation.

espindola accepted this revision.Mar 26 2018, 11:31 AM

LGTM.

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

This revision is now accepted and ready to land.Mar 26 2018, 11:31 AM
ruiu added a comment.Mar 26 2018, 1:39 PM

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.

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 added a comment.Mar 26 2018, 2:37 PM

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.

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.

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 added a comment.Mar 26 2018, 4:28 PM

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.

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.

We can close this one, right?

andrewng abandoned this revision.Mar 27 2018, 8:08 AM

Abandoned in favour of D44923.