[PATCH] D129337: [llvm-objcopy][ELF] Add --set-section-type

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Documentation needs updating.



================
Comment at: llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp:686-688
+      auto It2 = Config.SetSectionType.find(Sec.Name);
+      if (It2 != Config.SetSectionType.end())
+        Sec.Type = It2->second;
----------------
As --set-section-flags can potentially change the type, we might want to add a specific test to document the behaviour that --set-section-type overrides whatever --set-section-flags does.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/set-section-attr-and-rename.test:15
 
-# RUN: not llvm-objcopy --rename-section=.foo=.bar --set-section-flags=.bar=alloc %t %t.2 2>&1 | FileCheck %s --check-prefix=SET-BAR
+# RUN: llvm-objcopy --rename-section=.foo=.bar --set-section-type=.foo=5 %t %t.2
+# RUN: llvm-readobj -S %t.2 | FileCheck %s --check-prefix=CHECK2
----------------
Would it not make sense to fold this into the above test-case, so that it tests all three of the --set-section-* options in one go?


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/set-section-type.test:20
+
+## sh_type is an uint32_t. There is no diagnostic for an overflow value.
+# RUN: llvm-objcopy --set-section-type=.foo=0x10000000a %t %t.2 2>&1 | count 0
----------------
I pronounce `uint32_t` as "you-int-thir-ty-two-tee", which would mean it should be "a" not "an". I'm curious if you say it differently?


================
Comment at: llvm/tools/llvm-objcopy/ObjcopyOptions.cpp:281
+static Expected<std::pair<StringRef, uint64_t>>
+parseSetSectionType(StringRef TypeValue) {
+  if (!TypeValue.contains('='))
----------------
I think you should be able to avoid the code duplication with `parseSetSectionAlignment` by factoring most of this code out into a separate function called both here and there.


================
Comment at: llvm/tools/llvm-objcopy/ObjcopyOptions.cpp:291
+        "bad format for --set-section-type: missing section name");
+  uint64_t NewAlign;
+  if (Split.second.getAsInteger(0, NewAlign))
----------------
If not refactoring, this local variable name is wrong.


================
Comment at: llvm/tools/llvm-objcopy/ObjcopyOptions.cpp:833
+  for (auto Arg : InputArgs.filtered(OBJCOPY_set_section_type)) {
+    Expected<std::pair<StringRef, uint64_t>> NameAndAlign =
+        parseSetSectionType(Arg->getValue());
----------------
Local variable needs renaming.


================
Comment at: llvm/tools/llvm-objcopy/ObjcopyOptions.cpp:850-854
+      return createStringError(
+          errc::invalid_argument,
+          "--set-section-type=%s conflicts with --rename-section=%s=%s",
+          SR.NewName.str().c_str(), SR.OriginalName.str().c_str(),
+          SR.NewName.str().c_str());
----------------
As this is quite verbose, it might be good to factor it into a helper function or lambda, to avoid duplication with the set section flags case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129337