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.
| 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. | |
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. | |
| 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 | |
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?
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.
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.
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.