Page MenuHomePhabricator

Add --undefined-glob which is an --undefined with wildcard pattern match.
ClosedPublic

Authored by ruiu on Jun 13 2019, 12:37 AM.

Details

Summary

This patch adds new command line option --undefined-glob to lld.
That option is a variant of --undefined but accepts wildcard
patterns so that all symbols that match with a given pattern are
handled as if they were given by -u.

-u foo is to force resolve symbol foo if foo is not a defined symbol
and there's a static archive that contains a definition of symbol foo.

Now, you can specify a wildcard pattern as an argument for --undefined-glob.
So, if you want to include all JNI symbols (which start with "Java_"), you
can do that by passing --undefined-glob "Java_*" to the linker, for example.

In this patch, I use the same glob pattern matcher as the version script
processor is using, so it does not only support * but also ? and [...].

Discussion thread: http://lists.llvm.org/pipermail/llvm-dev/2019-June/132961.html

Diff Detail

Repository
rL LLVM

Event Timeline

ruiu created this revision.Jun 13 2019, 12:37 AM
Herald added a project: Restricted Project. · View Herald Transcript
MaskRay added inline comments.Jun 13 2019, 1:15 AM
lld/ELF/Driver.cpp
1327 ↗(On Diff #204439)

clang-format.

here not -> here is not

1343 ↗(On Diff #204439)

Name.find_first_of("?*[")

ruiu updated this revision to Diff 204445.Jun 13 2019, 1:19 AM
  • address review comments
MaskRay accepted this revision.Jun 13 2019, 1:20 AM
This revision is now accepted and ready to land.Jun 13 2019, 1:20 AM

Looks good to me as well.

lld/ELF/Driver.cpp
1335 ↗(On Diff #204445)

Might be worth extracting this block, and the equivalent one in handleUndefined into a separate function. That way the action to take to once a symbol has been selected by a pattern is always the same in both functions, including the comment about LTO. I've not got a strong opinion though as I don't expect much change.

ruiu updated this revision to Diff 204450.Jun 13 2019, 1:49 AM
  • factor out common code and unify two functions into one
grimar accepted this revision.Jun 13 2019, 2:04 AM

LGTM. Minor suggestion is inlined.

lld/test/ELF/undefined-wildcard.s
17 ↗(On Diff #204450)

hint: you should be able to use ----implicit-check-not=foo --implicit-check-not=bar
This way can probably help to make checks below a bit simpler.

Actually linker scripts do support metacharacter escaping (although, it seems lld does not support this correctly, yet).

E.g., this should match only a function named literally "?"

VER {
  global:
   "?";
};

While this matches any one-letter function:

VER {
  global:
   ?;
};

So perhaps this functionality should be given its own command-line argument, e.g. --undefined-glob, so as to not cause any such confusion.

ruiu added a comment.Jun 13 2019, 10:17 PM

Actually linker scripts do support metacharacter escaping (although, it seems lld does not support this correctly, yet).

E.g., this should match only a function named literally "?"

VER {
  global:
   "?";
};

While this matches any one-letter function:

VER {
  global:
   ?;
};

So perhaps this functionality should be given its own command-line argument, e.g. --undefined-glob, so as to not cause any such confusion.

I think I like the idea to add this as a new option, instead of changing the meaning of the existing option in a subtle way. I'll give it a new name as you suggested.

ruiu updated this revision to Diff 204699.Jun 13 2019, 10:40 PM
  • rename the option --undefined-glob.
ruiu retitled this revision from Support wildcard patterns in --undefined. to Add --undefined-glob which is an --undefined with wildcard pattern match..Jun 13 2019, 10:40 PM
ruiu edited the summary of this revision. (Show Details)
ruiu updated this revision to Diff 204700.Jun 13 2019, 10:41 PM
ruiu edited the summary of this revision. (Show Details)
  • update comment
MaskRay accepted this revision.Jun 14 2019, 12:08 AM

The code change looks good. Nit about the help text.

lld/docs/ld.lld.1
509 ↗(On Diff #204700)

I was puzzled when I first noticed this option and its help text: Force to be an undefined symbol during linking.

If the symbol does not exist, there is no undefined symbol in the link result.
If the symbol is defined, this option does not make the symbol undefined.

Now that you are adding a similar option, maybe revise the documentation a bit to address the confusion?

ruiu updated this revision to Diff 204744.Jun 14 2019, 4:49 AM
  • updated the man page
This revision was automatically updated to reflect the committed changes.