This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Teach ICF to take FDEs into account when doing code folding.
AbandonedPublic

Authored by grimar on Sep 27 2017, 8:46 AM.

Details

Reviewers
ruiu
rafael
Summary

ICF currently does not take in account data stored in FDEs.
That means if we have 2 "identical" code sections, but one
of them does not have frame data associated or both of them has
different data, code still be folded, what is not correct.

Patch does the first step to improve the situation. It teaches ICF
to compare data stored in FDEs as well, so that only sections
that have equal FDEs are eligible to be merged.

Diff Detail

Event Timeline

grimar created this revision.Sep 27 2017, 8:46 AM
grimar updated this revision to Diff 116824.Sep 27 2017, 8:48 AM
  • Removed unrelated change.
ruiu added inline comments.Sep 27 2017, 8:16 PM
ELF/EhFrame.cpp
67

I wonder if you actually tried this.

Do you really have to care about padding?

ELF/ICF.cpp
285

Rename equalsFde

ELF/InputSection.h
341

I strongly doubt if this is correct because the relationship between input sections and fdes is 1 to many.

grimar updated this revision to Diff 116944.Sep 28 2017, 2:04 AM
  • Addressed review comments.
ELF/EhFrame.cpp
67

Yes, please see eh-frame-hdr-icf-fde1.s testcase from this patch, it shows that 2 FDEs differs because of padding only.

ELF/ICF.cpp
285

Done.

ELF/InputSection.h
341

That is sure needs to be vector, but for start I decided to support simple case and improve it in following patches.
This patch open roads for other patches teaching ICF about handling .eh_frame data and it is already large (has 3 new testcases),
so to reduce amount of changes I supposed we can start from this ?

(FWIW having it as the simple pointer now does not break anything for us because before this patch ICF merged identical code ignoring all FDEs, but it already fixes some cases at the same time)

ruiu added inline comments.Oct 1 2017, 7:38 PM
ELF/EhFrame.cpp
67

I doubt if this is correct. There may be relocations pointing to given FDEs, and I think you need to compare these relocations too.

ELF/InputSection.h
341

I think I don't feel comfortable if you are going to check in a patch even though we know a correctness issue. Doing the right thing in this patch doesn't seem that hard.

grimar updated this revision to Diff 117319.Oct 2 2017, 3:39 AM
  • Addressed review comments.
ELF/EhFrame.cpp
67

This is correct I think, it just does not covers everything yet.
That is why I marked this patch as "first step" in description.

We need to scan relocations to compare LSDAs and also want to compare personality routines,
so want to take CIEs which contains a pointer to them in account when comparing FDEs during ICF.
That is what I want to implement after this patch.

ELF/InputSection.h
341

Done. Added testcase.

grimar updated this revision to Diff 118614.Oct 11 2017, 7:32 AM
  • Rebased.
ruiu added inline comments.Oct 24 2017, 1:43 PM
ELF/Driver.cpp
1104–1105

This change is dubious. ICF is not a part of writing a result, so it shouldn't be moved there.

ELF/EhFrame.cpp
67

I doubt if this is correct because this function seems to compare contents only byte-by-byte. You need to handle relocations right?

grimar updated this revision to Diff 120234.Oct 25 2017, 5:35 AM
  • Addressed review comments.
ELF/Driver.cpp
1104–1105

Ok. I moved createSyntheticSections and combineEhFrameSections before doIcf.

ELF/EhFrame.cpp
67

I have already answered the same question here:
https://reviews.llvm.org/D38319#inline-335138

ruiu added inline comments.Oct 25 2017, 7:33 AM
ELF/EhFrame.cpp
67

That URL just jumps to the top of the page to me.

grimar added inline comments.Oct 25 2017, 7:38 AM
ELF/EhFrame.cpp
67

Probably becaue this comment is hidden for you in phab view ? It was Mon, Oct 2, 1:39 PM:

"This is correct I think, it just does not covers everything yet.
That is why I marked this patch as "first step" in description.

We need to scan relocations to compare LSDAs and also want to compare personality routines,
so want to take CIEs which contains a pointer to them in account when comparing FDEs during ICF.
That is what I want to implement after this patch."

ruiu added inline comments.Oct 25 2017, 7:47 AM
ELF/EhFrame.cpp
67

I'd like to know how you'd scan relocations. If you have a patch, can you upload?

grimar added inline comments.Oct 25 2017, 7:48 AM
ELF/EhFrame.cpp
67

Not yet. Can try to do it and show you.

ruiu added inline comments.Oct 25 2017, 7:59 AM
ELF/EhFrame.cpp
67

It's worth doing. When I implement a feature, I often do it several times locally to see what is the best way of doing it.

grimar added inline comments.Nov 3 2017, 2:35 AM
ELF/EhFrame.cpp
67

I would like to clarify, do you think we can go in that direction for now (and then I'll try to update this with relocations scanning) or we would like to put this on hold for a while since honestly it is not clear for me with what "[RFC] Making .eh_frame more linker-friendly" can end up ?

ruiu edited edge metadata.Nov 14 2017, 1:17 AM

I don't think I want to add this much complexity to the linker to solve the problem that we need to split section contents that the compiler merged for no obvious reason. We should instead make the compiler not to merge sections in the first place. Thus the proposal.

grimar abandoned this revision.Nov 15 2017, 3:53 AM

Abandoning for now basing on recent comments.