Page MenuHomePhabricator

[llvm-objcopy] Add --add-symbol

Authored by evgeny777 on Feb 14 2019, 6:02 AM.

Diff Detail


Event Timeline

evgeny777 created this revision.Feb 14 2019, 6:02 AM
rupprecht added inline comments.Feb 14 2019, 11:11 AM
198 ↗(On Diff #186832)

I think these would be better as struct defaults, then just declare as "NewSymbolInfo SI;" here

202 ↗(On Diff #186832)

Can you elaborate on the bad format, e.g. for this case: missing: '='

207 ↗(On Diff #186832)

Here too

215 ↗(On Diff #186832)

Some things that I think we should consider:

  • This applies flags in order of seeing them, e.g. "file,section" is different than "section,file". Perhaps we should error on any flags that overlap?
  • GNU objcopy implements a few more: export, debug, constructor, warning, indirect, synthetic, indirect-function, unique-object, before=<other symbol>. We might want implement those as well, or at least accept them without error but ignore them.

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.

235 ↗(On Diff #186832)

Include the unsupported flag name, also, this should print the supported flag names.

66 ↗(On Diff #186832)


573 ↗(On Diff #186832)

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.

806 ↗(On Diff #186832)


243 ↗(On Diff #186832)

All of these could use some more docs:

  • Mention default behavior if section is not provided
  • Describe the format of value (number, hex, etc.)
  • Include all supported flags
evgeny777 marked 3 inline comments as done.Feb 15 2019, 12:35 AM
evgeny777 added inline comments.
215 ↗(On Diff #186832)

Perhaps we should error on any flags that overlap?

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.

66 ↗(On Diff #186832)

Like I said in D58173 this will not work, because uint64_t is not unsigned long long on 64 bit platforms

573 ↗(On Diff #186832)

The shndx parameter is ignored when DefinedIn is not nullptr. See addSymbol

evgeny777 updated this revision to Diff 186979.Feb 15 2019, 2:00 AM

Addressed some of review comments

jhenderson added inline comments.Feb 18 2019, 6:45 AM
217–221 ↗(On Diff #186979)

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 ↗(On Diff #186979)

Could you use a StringSwitch here, using CaseLower to get case insensitivity, perhaps with lambdas, to avoid the if/else list?

66 ↗(On Diff #186832)

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.

573 ↗(On Diff #186832)

This should probably be commented. It also feels rather fragile though.

jhenderson added inline comments.Feb 18 2019, 7:00 AM
66 ↗(On Diff #186832)

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.

jhenderson added inline comments.Feb 18 2019, 7:09 AM
238 ↗(On Diff #186979)

The return value here is unchecked. What happens if Value is not a valid number?

rupprecht added inline comments.Feb 19 2019, 3:52 PM
76 ↗(On Diff #187353)

nit: add a newline

251 ↗(On Diff #187353)

These noops can be combined with CasesLower("debug", "constructor", ...)

215 ↗(On Diff #186832)

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.

jhenderson added inline comments.Feb 20 2019, 2:00 AM
237 ↗(On Diff #187525)

See my comment below about returning Error rather than calling error.

295 ↗(On Diff #187525)

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 ↗(On Diff #187525)

Could this be folded in with the other ones below?

evgeny777 marked an inline comment as done.Feb 20 2019, 2:34 AM
evgeny777 added inline comments.
320 ↗(On Diff #187525)

CasesLower supports up to 5 string parameters. Are you suggesting to add another variant which takes 6 ones?

jhenderson added inline comments.Feb 20 2019, 3:59 AM
320 ↗(On Diff #187525)

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.

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.

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

rupprecht accepted this revision.Feb 22 2019, 10:14 AM
rupprecht added inline comments.
68 ↗(On Diff #187940)


242 ↗(On Diff #187940)

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.

This revision is now accepted and ready to land.Feb 22 2019, 10:14 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 25 2019, 6:12 AM