Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Can you check (or did you already?) whether this matches how GNU objcopy behaves (for the cases in GNU objcopy that seem intentional/sensible at least)?
Yes, I verified that mingw objcopy produces the same results for all tests from llvm/test/tools/llvm-objcopy/COFF/set-section-flags.test except tests error messages and new "exclude" flag. But I did not check what flags it preserves .
Ok - as @MaskRay asked about that, can you test which flags are preserved and which dropped implicitly as well?
As far as I see it does not preserve any flags at all (except alignment flags). Do you suggest to do the same in llvm-objcopy?
The new exclude flag is pretty ELF-centric as things stand. I don't know if there's an equivalent COFF section characteristic that really maps to it. If not, we should either a) reject it, or b) accept it and do nothing with it except perhaps clear the flags. I tentatively prefer b) for simplicity, but regardless it should at least be tested. What do others think?
Documentation should be updated so that --set-section-flags is no longer listed as ELF-specific.
llvm/test/tools/llvm-objcopy/COFF/set-section-flags.test | ||
---|---|---|
4 | Use '##' for comments to make them stand out better from lit/FileCheck directives. This is what we've tended to do in other new tests for readability. | |
llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp | ||
220 | Any particular reason you're making this const? |
I believe exclude flag which maps to SHF_EXCLUDE for ELF objects maps well to the COFF's IMAGE_SCN_LNK_REMOVE flag.
Documentation should be updated so that --set-section-flags is no longer listed as ELF-specific.
I agree. If documentation says that --set-section-flags is ELF specific it should definitely be updated.
llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp | ||
---|---|---|
220 | Nothing specific, but I do not see any reasons why it cannot be const especially if we do not plan to modify pointee . Do you see any reasons why it cannot have const qualifier? |
llvm/docs/CommandGuide/llvm-objcopy.rst | ||
---|---|---|
440–441 | These two lines need reflowing. We tend to try to keep lines within 80 characters in this document. | |
440–441 | This is still under the "ELF-specific Options" section... | |
444 | I feel like we should also document the effects on COFF objects. Otherwise, people aren't going to know what each setting does. | |
llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp | ||
220 | No specific reason, except that we don't often make local variables const from my experience. |
llvm/docs/CommandGuide/llvm-objcopy.rst | ||
---|---|---|
440–441 | Right. I did not realize that there is a dedicated ELF-specific section. I have added --set-section-flags option description to the COFF-specific section. |
llvm/docs/CommandGuide/llvm-objcopy.rst | ||
---|---|---|
440–441 | I've actually just removed the COFF-specific section (see rG0298a8751152). The normal approach would be to put the whole lot in the generic section (since the switch applies to multiple formats), and put the individual ELF and COFF bits one below the other in the description. See --strip-all for a sort-of example (it explains ELF and then COFF/Mach-O). |
Okay, LGTM, but I'd like somebody with more COFF familiarity to confirm they're happy.
I'd probably rephrase this as follows:
"For ELF objects, the flags have the following effects:"
I'd also rephrase the COFF one similarly.