[PATCH] D90438: [llvm-objcopy] Make --set-section-flags work with --add-section

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D90438#2368556 <https://reviews.llvm.org/D90438#2368556>, @MaskRay wrote:

> In D90438#2367654 <https://reviews.llvm.org/D90438#2367654>, @jhenderson wrote:
>
>> I think it's unlikely any user is ever going to want to change the flags of .gnu_debuglink. I think there's a slightly higher chance that they might want to for .symtab, which could potentially be added later on. However, I don't see any particular reason to prevent it - in this case, I'd consider GNU's behaviour unhelpful as if somebody has explicitly asked to change the flags of one of these sections, they presumably actually want to and ignoring it is unhelpful. Consequently, where we can //easily// support it, we should (in this case, I believe it's just move the new code even later in the function), but I don't think it's worth doing if it's hard to support.
>
> I know what you are getting at: `objcopy -R .symtab --add-symbol newsym=1234 --set-section-flags=.symtab=exclude a a2` => the flags of .symtab are unchanged. This may be due to BFD's special handling of .symtab
> It is probably indeed an unhelpful behavior.
>
> In this patch I don't want to touch that area:/ Changing the flags of .symtab is still an unlikely operation. `--add-sections .* set-section-flags` is more common.

My only change request was to just move the code you're moving in this patch to later in the same function. It's no more a complicated a change than the change you are doing already. This may not give all the support necessary, but it should make it easier to add support later on, if the need arises. See inline comment for the concrete location I'm suggesting. If it does start working at that point, you might want a test, but I don't mind too much either way (since as you say it would be a rare behaviour).



================
Comment at: llvm/test/tools/llvm-objcopy/ELF/add-section-and-set-flags.test:15-16
+
+---
+!ELF
+FileHeader:
----------------
Tends to be more common in the YAML I've seen elsewhere.


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:745-754
+  // --set-section-flags works with sections added by --add-section.
+  if (!Config.SetSectionFlags.empty()) {
+    for (auto &Sec : Obj.sections()) {
+      const auto Iter = Config.SetSectionFlags.find(Sec.Name);
+      if (Iter != Config.SetSectionFlags.end()) {
+        const SectionFlagsUpdate &SFU = Iter->second;
+        setSectionFlagsAndType(Sec, SFU.NewFlags);
----------------
I suggest moving this block to...


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:773
   }
 
   if (Config.EntryExpr)
----------------
... here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90438