[PATCH] D132051: [BOLT] Track fragment info for all split fragments

[Rafael Auler via Phabricator via llvm-commits]

rafauler accepted this revision.
rafauler added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: bolt/lib/Core/FunctionLayout.cpp:207-212
+  // Keep the main fragment around, because it may contain emission information
+  // even if the function is ignored.
+  std::for_each(Fragments.begin() + 1, Fragments.end(),
+                [](FunctionFragment *const FF) { delete FF; });
+  Fragments = FragmentListType{Fragments.front()};
+  getMainFragment().Size = 0;
----------------
FPar wrote:
> rafauler wrote:
> > Can you explain why was that added a bit better?
> Better?
Yes, thanks


================
Comment at: bolt/lib/Rewrite/RewriteInstance.cpp:4477-4491
+        if (FF.getAddress()) {
+          ELFSymTy NewColdSym = FunctionSymbol;
+          const SmallString<256> SymbolName = formatv(
+              "{0}.cold.{1}", cantFail(FunctionSymbol.getName(StringSection)),
+              FF.getFragmentNum().get() - 1);
+          NewColdSym.st_name = AddToStrTab(SymbolName);
+          NewColdSym.st_shndx =
----------------
FPar wrote:
> rafauler wrote:
> > Can we add a simple test to check that this is working as intended?
> > 
> > Mistakes in the symbol table update are nasty because the program will not crash and we may take a long time to detect (since it takes a human to read the symbol table and see that it doesn't make sense). For this case, a test is a good way of making sure this code won't break in the future.
> I have added a test at `bolt/test/X86/fragmented-symbols.s` in D132052 (easier to test in this diff). Does that cover what you were thinking of?
Yes, that's great, thanks


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132051/new/

https://reviews.llvm.org/D132051