honggyukim / uftrace

Function graph tracer for C/C++/Rust/Python
https://uftrace.github.io/slide/
GNU General Public License v2.0
1 stars 0 forks source link

Support for tracing the mcount function on RISC-V 64-bit architectures #18

Closed gichoel closed 1 year ago

gichoel commented 1 year ago

This is a porting effort to support tracing with the mcount function in the RISC-V 64-bit architecture environment.

honggyukim commented 1 year ago

Hi @gichoel,

Thanks very much for your work! I will first ask you to change the commit message titles as a first step.

The commit titles need to be changed because "for riscv64 architecture" part is unnecessarily repeated for every commits.

Then the commit messages can be changed from

arch: Update 'arch/riscv64/mcount.S' for riscv64 architecture
arch: Update 'cmds/record.c' for riscv64 architecture
Arch: Add 'arch/riscv64/plthook.S' for riscv64 architecture
arch: Add 'arch/riscv64/mcount-support.c' for riscv64 architecture
arch: Update 'utils/compiler.h' for riscv64 architecture
arch: Add 'arch/riscv64/mcount.S' for riscv64 architecture
arch: Add 'arch/riscv64/cpuinfo.c' for riscv64 architecture
arch: Add 'arch/riscv64/Makefile' for riscv64 architecture
arch: Add 'arch/riscv64/mcount-arch.h' for riscv64 architecture

to

arch/riscv64: Update 'arch/riscv64/mcount.S'
arch/riscv64: Update 'cmds/record.c'
arch/riscv64: Add 'arch/riscv64/plthook.S'
arch/riscv64: Add 'arch/riscv64/mcount-support.c'
arch/riscv64: Update 'utils/compiler.h'
arch/riscv64: Add 'arch/riscv64/mcount.S'
arch/riscv64: Add 'arch/riscv64/cpuinfo.c'
arch/riscv64: Add 'arch/riscv64/Makefile'
arch/riscv64: Add 'arch/riscv64/mcount-arch.h'

However, now the titles only show "Add" and "Update". I think those messages do not provide any useful information.

Please rewrite the commit title and messages then I will have a look inside again.

In addition, the last commit e8cd34bb36802d26a2856f276a772d65bd9115a4 doesn't have to be in a separate patch so please squash it into a proper commit.

gichoel commented 1 year ago

Hmm... That's very strange.

Because I had already added the skeleton structure to the part of the commit called arch/riscv64: Update 'utils/compiler.h' to fix compilation errors

@honggyukim I believe this happened for the following reasons.

A commit from mirusu400 came in in the middle of this PR, and after thinking about what could have gone wrong, I finally remembered that the commit time of "utils/compiler.h" was later than the rest of the code.

So I cloned the point in time before mirusu400 pushed "utils/compiler.h", and it looks like "utils/compiler.h" disappeared because mirusu400 pushed his work here.

I 'git push --force' the re-commit in my local environment, so it was fine.

gichoel commented 1 year ago

I will reapply and upload the feedback regarding annotations along with the fix for mcount.S that we previously communicated.

gichoel commented 1 year ago

Hmmm... I still don't have a clear understanding of git bisect, but I understand that it's used to find when things aren't working as they should (e.g. build failures).

Also, I think what I missed is that all commits must individually succeed in the build, but the commits for the RISC-V porting must be reflected until the last one to succeed in the build.

I'll fix the commits to reflect this and re-upload them.

honggyukim commented 1 year ago

You must properly rebase your patches. The current PR brings many other unrelated commits together.

gichoel commented 1 year ago

There's still a lot I don't know about git, so I didn't know why someone else's commit went up, but I think I know now.

The problem was that I had run a git rebase master to reflect the latest commits reflected in master, and in the process had pulled commits ahead of the current arch/riscv64 branch.

So at this point I was thinking of closing the PR and reposting it to namhyung/uftrace as we discussed before, would that be okay?

honggyukim commented 1 year ago

Yes, please upload a PR in the @namhyung's upstream uftrace repo.