[PATCH] D122432: [lld-macho] Consolidate all priority assignments under `buildInputSectionPriorities()`

[Roger Kim via Phabricator via llvm-commits]

Roger created this revision.
Herald added projects: lld-macho, All.
Herald added a reviewer: lld-macho.
Roger updated this revision to Diff 418116.
Roger added a comment.
Roger retitled this revision from "[lld-macho][NFC] Bring together all priority assignments into one place." to "[lld-macho] Consolidate all priority assignments under `buildInputSectionPriorities()`".
Roger edited the summary of this revision.
Roger updated this revision to Diff 418233.
Roger updated this revision to Diff 418242.
Roger edited the summary of this revision.
Roger updated this revision to Diff 418732.
Roger published this revision for review.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Update


Roger added a comment.

Rebase


Roger added a comment.

add comment


Roger added a comment.

Ready for review



================
Comment at: lld/MachO/SectionPriorities.cpp:319
       if (fromSym && toSym &&
-          (!hasOrderFile ||
-           (!getSymbolPriority(fromSym) && !getSymbolPriority(toSym))))
+          (!hasInputOrder ||
+           (!isOrdered(*fromSym, obj->getName().str(), inputOrder) &&
----------------
honestly, this condition feels a little weird to me because this boolean can short circuit the expression and skip checking for symbol priority, but this also skips checking if the symbol is absolute/not.

I'm just retaining the existing logic, but I question the correctness of the existing logic.


The previous code assigned priorities for sections across different call stacks from Driver.cpp. The data gathered to assign priorities seems to be spread across different call stacks by necessity (test fails when I try to put them all together). However, the actual priority assignments do not need to be. After all of the relevant data has been collected, we can make all the priority assignments under one function (ie, `buildInputSectionPriorities()`) specific for that purpose. This is the change for this diff.

This diff is almost completely a NFC but there is one slight functional change that I believe corrects a bug. The existing code for `extractCallGraphProfile()` filters data about calls from or to symbols that already has a priority (based on the order file). There are two ways for a symbol to have a priority at that point in time:

1. The symbol is specifically referenced by name and its order file in the order file.
2. The symbol is indirectly referenced by an entry in the order file that contains its name but specifies no order file.

If two order files each have a symbol using the same name and the order file includes one of the symbols (specifying its order file), the existing code produced a side effect of giving all symbols of the same name a default priority of zero. Having a priority of zero is not the same as having no priority at all as now those symbols with default priority zero will have their calls ignored by `extractCallGraphProfile()`.

My diff changes this behavior so that an entry in the order file for one symbol in a specific object file will have no effect on the priority of symbols with the same name in other object files.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D122432

Files:
  lld/MachO/Driver.cpp
  lld/MachO/SectionPriorities.cpp
  lld/MachO/SectionPriorities.h

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D122432.418732.patch
Type: text/x-patch
Size: 8280 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220328/fa1ba237/attachment.bin>