[PATCH] D67656: [llvm-objcopy] Add --set-section-alignment

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked an inline comment as done and an inline comment as not done.
MaskRay added inline comments.


================
Comment at: tools/llvm-objcopy/ELF/ELFObjcopy.cpp:673
 
+  if (!Config.SetSectionAlignment.empty()) {
+    for (SectionBase &Sec : Obj.sections()) {
----------------
abrachet wrote:
> Is this idea behind not putting this and the SetSectionFlags loop that does the same thing in the same loop because they are unlikely to be used? Without looking very far I see 4 times including here that we go through every section.
> 
> Not that it makes sense for this patch but it might eventually be worth it to hash every section by name into a `StringMap` so we only iterate once over all sections and can handle `SetSectionAlignment`, `SetSectionFlags` and `SectionsToRename` more efficiently.
I think the time is dominated by the hash table lookup `SetSectionAlignment.find`, not the loop itself. So separate loop is probably better if we unlikely do lots of different operations in the same invocation of llvm-objcopy.

The same strategy is used by replaceAndRemoveSections. Though, I am not there it is the best there. The updateAndRemoveSymbols way may be simpler. I'd like to explore whether regrouping some operations may make the logic clearer there.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67656