Details
Diff Detail
Event Timeline
tools/llvm-objcopy/CopyConfig.cpp | ||
---|---|---|
197 | I think these would be better as struct defaults, then just declare as "NewSymbolInfo SI;" here | |
201 | Can you elaborate on the bad format, e.g. for this case: missing: '=' | |
206 | Here too | |
214 | Some things that I think we should consider:
Take a look at parseSectionRenameFlag above and see if some of that pattern helps -- it at least helps for ordering, i.e. do one pass to get parsed flag values, then another pass to apply parsed flags to bind/visibility/type. | |
234 | Include the unsupported flag name, also, this should print the supported flag names. | |
tools/llvm-objcopy/CopyConfig.h | ||
67 | uint64_t | |
tools/llvm-objcopy/ELF/ELFObjcopy.cpp | ||
573 | GNU docs says: "If the section is given, the symbol will be associated with and relative to that section, otherwise it will be an ABS symbol." I think SHN_ABS is wrong if the section name is given. | |
tools/llvm-objcopy/ELF/Object.h | ||
806 | SecIt->get() | |
tools/llvm-objcopy/ObjcopyOpts.td | ||
243 | All of these could use some more docs:
|
tools/llvm-objcopy/CopyConfig.cpp | ||
---|---|---|
214 |
I checked parseSectionRenameFlag and looks like it also doesn't check for overlaps (you can pass both code and data, load and noload, etc). FYI GNU objcopy also doesn't check for overlaps. | |
tools/llvm-objcopy/CopyConfig.h | ||
67 | Like I said in D58173 this will not work, because uint64_t is not unsigned long long on 64 bit platforms | |
tools/llvm-objcopy/ELF/ELFObjcopy.cpp | ||
573 | The shndx parameter is ignored when DefinedIn is not nullptr. See addSymbol |
tools/llvm-objcopy/CopyConfig.cpp | ||
---|---|---|
217–221 | I'd fold this bit of the comment in with the <flags> bit above, since they are closely linked. Also, I'd remove the word "silently" from it. | |
242 | Could you use a StringSwitch here, using CaseLower to get case insensitivity, perhaps with lambdas, to avoid the if/else list? | |
tools/llvm-objcopy/CopyConfig.h | ||
67 | I don't understand what you mean by this? Symbol values are by definition in the ELF spec up to 64-bit in size, so uint64_t (which is 64-bits in size) is the correct type. unsigned long long is only guaranteed to be at least as big as unsigned long which in turn is only guaranteed to be as big as unisgned int etc. Related note: The bitness of the platform does not define the size of types by itself. For example, on Windows, unsigned long is always 32 bits, both for 32-bit Windows and 64-bit Windows. On the other hand, unsigned long long is 64 bits on both. | |
tools/llvm-objcopy/ELF/ELFObjcopy.cpp | ||
573 | This should probably be commented. It also feels rather fragile though. |
tools/llvm-objcopy/CopyConfig.h | ||
---|---|---|
67 | Okay, I now see what the problem is. It's to do with the call to getAsUnsignedInteger, later on right? I think this issue could be solved by defining an unsigned long version of getAsUnsignedInteger. That way, it should work regardless of the underlying type of uint64_t. Implementing that would be my preference to working around this by using primitive types in llvm-objcopy rather than fixed-width types. |
tools/llvm-objcopy/CopyConfig.cpp | ||
---|---|---|
238 | The return value here is unchecked. What happens if Value is not a valid number? |
test/tools/llvm-objcopy/ELF/add-symbol.test | ||
---|---|---|
74 | nit: add a newline | |
tools/llvm-objcopy/CopyConfig.cpp | ||
214 | parseSectionFlagSet (the caller of parseSectionRenameFlag) doesn't check for overlap, but it doesn't look like any active features overlap -- e.g. "load" and "noload" don't do anything (they're just for GNU compatability), so it doesn't error out on details that don't matter. I don't think GNU objcopy is being helpful if it permits overlaps -- if the user types "hidden,default" (maybe thinking "default" indicates "default binding type"), they might be surprised when the visibility is not hidden. | |
251 | These noops can be combined with CasesLower("debug", "constructor", ...) |
tools/llvm-objcopy/CopyConfig.cpp | ||
---|---|---|
237 | See my comment below about returning Error rather than calling error. | |
295 | I think we're generally aiming to move away from error/reportError in favour of passing Error/Expected back up the stack to be handled at the top level. Please do that instead here and elsewhere where you've added new error messages. You might need to rebase on top of D58316. | |
320 | Could this be folded in with the other ones below? |
tools/llvm-objcopy/CopyConfig.cpp | ||
---|---|---|
320 | CasesLower supports up to 5 string parameters. Are you suggesting to add another variant which takes 6 ones? |
tools/llvm-objcopy/CopyConfig.cpp | ||
---|---|---|
320 | Ah, I didn't realise that. This is fine then. |
Sorry to be the Grinch again but what is the use case for this? This isn't a feature in GNU objcopy as far as I can tell and the semantics of such a change are quite involved. I think I'd prefer to discuss this in llvm-dev first. I belive this can be given meaningful and consistent semantics and I think it could have a use case but right now I don't see one. This seems like a complicated feature to add without a use case in mind.
Not OP (and I don't have a use case), but from man objcopy:
--add-symbol name=[section:]value[,flags] Add a new symbol named name while copying the file. ...
Sorry to be the Grinch again but what is the use case for this?
Haha, no that's in fact a good question =)
I'm using it to hack on a stripped binary. You can add symbols here and there and then set breakpoints with gdb.
Makes things a little bit easier
tools/llvm-objcopy/CopyConfig.h | ||
---|---|---|
67 | uint64_t | |
tools/llvm-objcopy/ObjcopyOpts.td | ||
242 | We should list all all the flags we support here, something like: "Add new symbol <name> to .symtab. Accepted flags: global, local, weak, default, hidden, file, section, object, function, indirect-function. Accepted but ignored for compatibility: debug, constructor, warning, indirect, synthetic, unique-object, before." I think string you have is actually fine for --help, but we don't have a man page to put the more verbose info in, so we should include it in --help so we have it somewhere. We should probably create a man page for llvm-objcopy in the llvm-9 release and make these docs simpler. |
nit: add a newline