This is an archive of the discontinued LLVM Phabricator instance.

Make calls into the pthread library use weak symbols.
ClosedPublic

Authored by saugustine on Apr 4 2019, 2:10 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

saugustine created this revision.Apr 4 2019, 2:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 4 2019, 2:10 PM

What problem are you trying to solve with this?

I think we talked about it already in a different setting, but why not implement a no-op RWMutex instead?

This is better than a no-op RWMutex because then the same binary works both when the application is multi-threaded and when it isn't. Therefore the build doesn't have to distinguish between the two cases.

ldionne added inline comments.Apr 5 2019, 11:43 AM
libunwind/src/RWMutex.hpp
62 ↗(On Diff #193780)

When you redeclare functions like this, are attributes stacked with the attributes of other (previous) declarations?

73 ↗(On Diff #193780)

Just to make sure I understand: pthread_create != nullptr is only going to be satisfied if pthread_create was linked into the program, _and_ it was used at least once before? I thought it would be non-null whenever it was linked into the program, whether you used it or not.

saugustine marked 2 inline comments as done.Apr 5 2019, 1:51 PM
saugustine added inline comments.
libunwind/src/RWMutex.hpp
62 ↗(On Diff #193780)

The gcc documentation says so.

6.33 Declaring Attributes of Functions
... Compatible attribute specifications on distinct declarations of the same function are merged....

The clang docs aren't specific on this point. But I know of enough uses of this style that I would consider it a bug if it didn't.

73 ↗(On Diff #193780)

It will be non-null only if it was linked. Whether or not it was used makes no difference.

This is more conservative than strictly necessary, but it saves the complexity of tracking whether or not multiple threads are actually active.

E5ten added a subscriber: E5ten.Apr 5 2019, 2:43 PM

@kledzik

Nick, what do you think about adding weak symbols just for achieving this?

@kledzik

Nick, what do you think about adding weak symbols just for achieving this?

Nick any thoughts? I'd like to move forward, for whatever definition of "forward" we may settle on.

mtrent added a subscriber: mtrent.Apr 18 2019, 1:23 PM

@kledzik

Nick, what do you think about adding weak symbols just for achieving this?

Nick any thoughts? I'd like to move forward, for whatever definition of "forward" we may settle on.

Nick is out of the office this week. He will be back next week.

Hi Nick--hope your vacation was good.

When you get a chance, would you respond to ldionne's question above?

Can this be conditionalized to not happen on macOS/iOS? I can see needing this on platforms where pthreads is optional, but it looks like this code will apply to macOS too, which introduces risk there.

Given this, I think implementing a no-op RWMutex is a better solution. And if you need the ability for the same library to work in both cases, maybe this code could be in a WeakRWMutex (or something like that).

saugustine updated this revision to Diff 198718.May 8 2019, 1:39 PM
  • Add an option to make pthread symbol references weak.

I've now put the weak-reference version behind a non-default build option. Is this acceptable?

ldionne added inline comments.May 9 2019, 12:19 PM
libunwind/src/CMakeLists.txt
70 ↗(On Diff #198718)

This is not an existing CMake option, is it? You should add it.

saugustine updated this revision to Diff 198926.May 9 2019, 3:38 PM
  • Make LIBUNWIND_WEAK_PTHREAD a formal Cmake option.
saugustine marked an inline comment as done.May 9 2019, 3:38 PM
ldionne accepted this revision.May 10 2019, 9:26 AM
This revision is now accepted and ready to land.May 10 2019, 9:26 AM

Any of the previous reviewers have a comment before I commit?

This revision was automatically updated to reflect the committed changes.