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

Add Big-endian PowerPC code coverage measurement #1679

Closed
briansmith opened this issue Oct 2, 2023 · 9 comments
Closed

Add Big-endian PowerPC code coverage measurement #1679

briansmith opened this issue Oct 2, 2023 · 9 comments

Comments

@briansmith
Copy link
Owner

briansmith commented Oct 2, 2023

Right now neither 32-bit nor 64-bit PowerPC code coverage is being measured in CI because neither target supports the profiler builtins.

powerpc64le-unknown-linux-gnu does support the profiler builtins.

Also @uweigand added profiler builtins to the s390x-unknown-linux-gnu in rust-lang/rust#104304, so big-endian support is already there.

/cc @runlevel5 @ecnelises @q66 @erichte-ibm

@briansmith briansmith changed the title Add Big-endian PowerPC code coverage Add Big-endian PowerPC code coverage measurement Oct 6, 2023
@briansmith
Copy link
Owner Author

briansmith commented Oct 18, 2023

Hi friends, would it be possible to make progress on this? Would it be helpful if I filed the issues in the rust-lang/rust repo for adding the profiler builtins for these targets? I am planning to file an issue asking that having working profiler builtins be a requirement for all "Tier 2" targets for Rust.

@briansmith
Copy link
Owner Author

I am planning to file an issue asking that having working profiler builtins be a requirement for all "Tier 2" targets for Rust.

rust-lang/rfcs#3517

@ecnelises
Copy link
Contributor

Big endian PowerPC on Linux is not so popular today. But let me check it inside QEMU first.

@ecnelises
Copy link
Contributor

I enabled profiler and cross built rust compiler for powerpc64-unknown-linux-gnu successfully on a ppc64le machine. I will try build for powerpc/powerpc64-linux and verify compiling programs with profile-generate and profile-use. IIUC that is all the necessary test?

BTW the CI Dockerfiles for powerpc and powerpc64 seems outdated. I may need to do some update.

@briansmith
Copy link
Owner Author

You can try the equivalent of RING_COVERAGE=1 mk/cargo.sh +nightly --target={YOUR_TARGET} test to see if code coverage measurement works. This basically adds -Cinstrument-coverage to $RUSTFLAGS and -fprofile-instr-generate -fcoverage-mapping to $CFLAGS, as well as setting $CC to clang. (You probably have to set clang as the linker as well.) If this builds and runs the tests then it should generate coverage output in which case it is probably exactly what I need.

@briansmith
Copy link
Owner Author

@ecnelises Any chance we may see a PR for the code coverage soon? I do believe it is just a matter of adding --enable-profiler to the Docker file? I understand you also think the Docker file should get updated in other ways but I wonder if we could do this minimal change. or maybe it's not sufficient?

I get nervous about supporting these big-endian targets when BoringSSL explicitly doesn't, especially when they keep reinforcing that, e.g. https://boringssl.googlesource.com/boringssl/+/ec87e1a7672044cbd7d484f2478cd618a7f1f81a%5E%21/#F0. Getting the code coverage measurement working would help alleviate those concerns.

@ecnelises
Copy link
Contributor

Sorry for so late response! I tested cross building a Rust toolchain targeting ppc64-unknown-linux-gnu and ran mk/cargo.sh test with coverage. It went well until last step SIGILL in QEMU, but I believe that's environment issue. I will try on another machine and do update this week.

Per my understanding, I should add powerpc64-unknown-linux-gnu (and 32-bit one) into CI config profiling builtin section of this repo, after adding profiler builtin to Rust Dockerfile build flags?

@briansmith
Copy link
Owner Author

Sorry for so late response! I tested cross building a Rust toolchain targeting ppc64-unknown-linux-gnu and ran mk/cargo.sh test with coverage. It went well until last step SIGILL in QEMU, but I believe that's environment issue. I will try on another machine and do update this week.

Great!

Per my understanding, I should add powerpc64-unknown-linux-gnu (and 32-bit one) into CI config profiling builtin section of this repo, after adding profiler builtin to Rust Dockerfile build flags?

That would be awesome. You should be able to copy the powerpc64le-unknown-linux-gnu entries. It's easy for either you or me to do.

Getting the profiler builtins working in rust-lang/rust and testing it is the part I could use the most help with.

@briansmith
Copy link
Owner Author

PR #1874 added this for powerpc64, thanks to @ecnelises. PR #1877 is attempting to do the same for 32-bit PowerPC but it is blocked on rust toolchain issues; see that PR for details. Regardless, I'm going to close this issue since the PR tracks it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants