Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use platform-dependent mcount function #57244

Closed
wants to merge 1 commit into from
Closed

Use platform-dependent mcount function #57244

wants to merge 1 commit into from

Conversation

quark-zju
Copy link
Contributor

Follow up with @nagisa's comment on #57220. Not all platforms use the
mcount name. Added a test.

Follow up with @nagisa's comment on #57220. Not all platforms use the
`mcount` name. Added a test.
@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 1, 2019
const_cstr!("_mcount")
} else {
const_cstr!("mcount")
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not seem to cover it.

Looking at clang’s this->MCountName instances:

  • darwin uses \01mcount;
  • On Aarch64 if abi is gnueabi, \01_mcount is used;
  • On ARM-linux or ARM-unknown if abi is gnueabi \01__gnu_mcount_nc is used and \01mcount if not gnueabi;

Please make this a target property (specified in target files) and use that property here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I used github search like this. I’m sure it missed some cases as well)

@petrochenkov
Copy link
Contributor

r? @nagisa

@rust-highfive rust-highfive assigned nagisa and unassigned petrochenkov Jan 1, 2019
@quark-zju
Copy link
Contributor Author

Thanks for the comments! I was aware of the other mcount names. I didn't add the \01 prefix as I don't fully understand it - When using objdump on an OSX clang -pg binary, it seems \01 prefix got stripped. The clang test didn't test them either (they probably should).

Meanwhile I'm on vacation and have temporarily lost access to fast network, fast machines, and osx platforms. I'll continue work on this when I regain access.

@namhyung
Copy link

namhyung commented Jan 3, 2019

I also don't know why the \01 prefix is there. But the gnueabi names should be used when it meets the requirements from the ABI. The mcount function in glibc has two arguments - current and parent function addresses. It's architecture dependent how these info will be passed (and be restored if needed).

Please take a look at below:

https://github.com/torvalds/linux/blob/master/arch/arm/kernel/entry-ftrace.S

The tools using the mcount facility (including uftrace) will expect same stack/register layout from the name.

@nagisa
Copy link
Member

nagisa commented Jan 3, 2019

I didn't add the \01 prefix as I don't fully understand it - When using objdump on an OSX clang -pg binary, it seems \01 prefix got stripped.

It is usually the case that tools like objdump will demangle the ABI-related symbols.

In any case, I would recommend to just add a target property for now, without worrying about what value would be the most correct for the target. Change only the targets where you’re confident about the difference and leave the other platforms with a default of some sort (e.g. mcount). Target maintainers will end up dealing with it for their target as/if any issues come up.

@Dylan-DPC-zz Dylan-DPC-zz added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 21, 2019
@Dylan-DPC-zz
Copy link

ping from triage @quark-zju any updates?

@quark-zju
Copy link
Contributor Author

@Dylan-DPC I'm still on vacation. I'll get back to this next week if everything goes well.

@TimNN TimNN added A-allocators Area: Custom and system allocators and removed A-allocators Area: Custom and system allocators labels Jan 29, 2019
@Dylan-DPC-zz
Copy link

ping from triage @quark-zju any updates on this?

@Centril
Copy link
Contributor

Centril commented Feb 23, 2019

ping from triage @quark-zju :)

@Dylan-DPC-zz
Copy link

ping from triage @quark-zju
Unfortunately we haven't heard from you on this in a while, so I'm closing the PR to keep things tidy. Don't worry though, if you'll have time again in the future please reopen this PR, we'll be happy to review it again! Thanks for taking the time to contribute :)

@Dylan-DPC-zz Dylan-DPC-zz added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 11, 2019
Centril added a commit to Centril/rust that referenced this pull request Mar 31, 2019
bors added a commit that referenced this pull request Mar 31, 2019
Centril added a commit to Centril/rust that referenced this pull request Mar 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants