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

gichoel · 2023-08-21T14:53:49Z

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

honggyukim · 2023-08-22T12:22:33Z

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 e8cd34b doesn't have to be in a separate patch so please squash it into a proper commit.

Fix a small typo in uftrace.md manpage. Signed-off-by: Seong Jin Kim <mirusu400@naver.com>

Retranslate the latest `uftrace.md` in Korean Signed-off-by: Seong Jin Kim <mirusu400@naver.com>

honggyukim

Thanks for the update.

You've already split the patches down into piece by piece logically. However, each patch must make the build successful. The current build fails at the initial patch.

$ make
make[1]: Nothing to be done for '/home/user/work/uftrace/arch/riscv64/uftrace.o'.
  LINK     uftrace
/usr/bin/ld: /home/user/work/uftrace/utils/perf.o: in function `setup_perf_record':
/home/user/work/uftrace/utils/perf.c:74: undefined reference to `read_memory_barrier'
/usr/bin/ld: /home/user/work/uftrace/utils/perf.c:72: undefined reference to `full_memory_barrier'
collect2: error: ld returned 1 exit status
make: *** [Makefile:336: /home/user/work/uftrace/uftrace] Error 1

I would suggest you to add skeleton structure so that the compilation done first at the initial patch, then build up the internal logic one by one.

In addition, I've left a few more comments below.

arch/riscv64/mcount-arch.h

honggyukim · 2023-08-24T22:56:34Z

utils/compiler.h

@@ -37,6 +37,14 @@
 #endif
 #endif

+//TODO: Requires implementation for RISC-V 64bit architecture


Do not use // comment, use /* ... */ comment instead.

In addition, please add an empty space after // so //TODO should be /* TODO: ... */.

arch/riscv64/mcount-support.c

gichoel · 2023-08-25T04:27:09Z

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

Reference link: 04cbf38

@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 · 2023-08-25T04:27:37Z

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

honggyukim

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

Your first commit doesn't have utils/compiler.h so the compilation fails. What I asked was to make each patch makes the build successful. The build is successful only when the e96ecba is introduced.

9bef6468 arch/riscv64: Resolve 'Unsupported macine ...' error output              <- build OK
e96ecbaa arch/riscv64: Add 'plthook.S' to resolve compile errors                  <- build OK
b8305343 arch/riscv64: Add 'mcount-support.c' to resolve compile errors           <- build fail
66d8f6db arch/riscv64: Update 'utils/compiler.h' to resolve compile errors        <- build fail
e96f5eed arch/riscv64: Add 'mcount.S' to support tracking based on mcount func    <- build fail
e7449a89 arch/riscv64: Add 'cpuinfo.c' for get architecture-dependent CPU Info    <- build fail
13d37f9d arch/riscv64: Add 'Makefile' for compiling architecture-dependent code   <- build fail
061e911f arch/riscv64: Add 'mcount-arch.h' for declare register info for an arch  <- build fail

But you don't have to change all the patches if you feel it's tricky to handle because anyway the riscv64 build fails even before having this PR.

If you think about doing git bisect in the future, you will understand why I asked this.

honggyukim · 2023-08-26T01:36:04Z

arch/riscv64/mcount-support.c

+	/*
+	* TODO:
+	*   This was implemented as a meaningless value because we focused on
+	*   successful builds with no compilation errors.
+	*
+	*   Therefore, it should be modified for future RISC-V 64-bit
+	*   architectures.
+	*/


Please fix the alignment for the comment.
Before:

/* * TODO: ... */

After:

/* * TODO: ... */

In addition, I think the same and long comment doesn't have to be repeated multiple times. You can just make it shorter as follows.

/* TODO: not implemented yet. */

Anyone can easily understand the meaning.

honggyukim · 2023-08-26T01:36:19Z

arch/riscv64/mcount-support.c

+	/*
+	* TODO:
+	*   This was implemented as a meaningless value because we focused on
+	*   successful builds with no compilation errors.
+	*
+	*   Therefore, it should be modified for future RISC-V 64-bit
+	*   architectures.
+	*/


honggyukim · 2023-08-26T01:36:26Z

arch/riscv64/mcount-support.c

+	/*
+	* TODO:
+	*   This was implemented as a meaningless value because we focused on
+	*   successful builds with no compilation errors.
+	*
+	*   Therefore, it should be modified for future RISC-V 64-bit
+	*   architectures.
+	*/


honggyukim · 2023-08-26T01:36:46Z

arch/riscv64/mcount-support.c

+	/*
+	* TODO:
+	*   This was implemented as a meaningless value because we focused on
+	*   successful builds with no compilation errors.
+	*
+	*   Therefore, it should be modified for future RISC-V 64-bit
+	*   architectures.
+	*/


honggyukim · 2023-08-26T01:36:52Z

arch/riscv64/mcount-support.c

+	/*
+	* TODO:
+	*   This was implemented as a meaningless value because we focused on
+	*   successful builds with no compilation errors.
+	*
+	*   Therefore, it should be modified for future RISC-V 64-bit
+	*   architectures.
+	*/


Signed-off-by: SangGyuLee <239lsg239@gmail.com>

The missing options -Z *SIZE*, \--size-filter=*SIZE*, -L LOCATION' '--loc-filter=LOCATION' and '--with-syms=DIR' are translated and added to man pages in Korean. Signed-off-by: Jeonghwan In <jhpc0128@naver.com>

The function add_trigger has been replaced with update_trigger, rendering it unnecessary. This commit removes the now-unused add_trigger function. Signed-off-by: Soyeon Park <sypark9646@gmail.com>

gichoel · 2023-08-27T04:10:37Z

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.

Revised the uftrace-dump.md manual to include the missing = sign in the description of the --depth option. Signed-off-by: SeokMin Kwon <ksm012015@naver.com>

This is a commit to ensure a successful build in a RISC-V 64bit environment, no functional behavior is implemented. Signed-off-by: Gichoel Choi <gichoel3101@gmail.com>

This is a commit to support tracing for mcount functioin in 'mcount.S'. Info : When compiling the target program with gcc, you need to add '-O0' or add '-fno-omit-frame-pointer' as an option because it will disable frame pointers when optimization options are enabled. Signed-off-by: Gichoel Choi <gichoel3101@gmail.com>

This is a commit to get CPU information using the '/proc/cpuinfo' file in a RISC-V 64bit environment. Signed-off-by: Gichoel Choi <gichoel3101@gmail.com>

honggyukim · 2023-08-28T12:51:11Z

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

gichoel · 2023-08-28T14:39:02Z

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 · 2023-08-29T11:10:19Z

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

gichoel force-pushed the arch/riscv-support-mcount branch from 1db036e to e8cd34b Compare August 21, 2023 14:59

gichoel force-pushed the arch/riscv-support-mcount branch 2 times, most recently from bff910e to 4e26849 Compare August 22, 2023 14:13

mirusu400 added 2 commits August 22, 2023 21:10

doc: Fix typo in uftrace.md

e4c290a

Fix a small typo in uftrace.md manpage. Signed-off-by: Seong Jin Kim <mirusu400@naver.com>

doc/ko: Update uftrace.md in Korean

4dd5dc0

Retranslate the latest `uftrace.md` in Korean Signed-off-by: Seong Jin Kim <mirusu400@naver.com>

honggyukim reviewed Aug 24, 2023

View reviewed changes

gichoel force-pushed the arch/riscv-support-mcount branch 2 times, most recently from c3a0da0 to 9bef646 Compare August 25, 2023 13:03

honggyukim reviewed Aug 26, 2023

View reviewed changes

jigsaw-0 and others added 3 commits August 26, 2023 11:29

doc/ko: Update CONTRIBUTING.md in Korean

152b901

Signed-off-by: SangGyuLee <239lsg239@gmail.com>

doc/ko: Add missing options in man pages

4c79abb

The missing options -Z *SIZE*, \--size-filter=*SIZE*, -L LOCATION' '--loc-filter=LOCATION' and '--with-syms=DIR' are translated and added to man pages in Korean. Signed-off-by: Jeonghwan In <jhpc0128@naver.com>

filter: Remove obsolete function add_trigger

d6f6edb

The function add_trigger has been replaced with update_trigger, rendering it unnecessary. This commit removes the now-unused add_trigger function. Signed-off-by: Soyeon Park <sypark9646@gmail.com>

doc: Add = sign for '--depth' option in uftrace-dump.md

5897da8

Revised the uftrace-dump.md manual to include the missing = sign in the description of the --depth option. Signed-off-by: SeokMin Kwon <ksm012015@naver.com>

gichoel force-pushed the arch/riscv-support-mcount branch 3 times, most recently from 83298ac to 0c2bf2a Compare August 27, 2023 15:22

gichoel added 3 commits August 28, 2023 00:58

arch/riscv64: Add basic build support for RISC-V64

daf345d

This is a commit to ensure a successful build in a RISC-V 64bit environment, no functional behavior is implemented. Signed-off-by: Gichoel Choi <gichoel3101@gmail.com>

arch/riscv64: Update 'cpuinfo.c' for get architecture-dependent cpu info

1b13502

This is a commit to get CPU information using the '/proc/cpuinfo' file in a RISC-V 64bit environment. Signed-off-by: Gichoel Choi <gichoel3101@gmail.com>

gichoel force-pushed the arch/riscv-support-mcount branch from 0c2bf2a to 1b13502 Compare August 27, 2023 15:58

gichoel closed this Aug 29, 2023

gichoel mentioned this pull request Aug 29, 2023

arch/riscv64: Add basic mcount tracing support for RISC-V 64bit namhyung/uftrace#1815

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

gichoel commented Aug 21, 2023

honggyukim commented Aug 22, 2023 •

edited

Loading

honggyukim left a comment

honggyukim Aug 24, 2023

gichoel commented Aug 25, 2023 •

edited

Loading

gichoel commented Aug 25, 2023

honggyukim left a comment

honggyukim Aug 26, 2023 •

edited

Loading

honggyukim Aug 26, 2023

honggyukim Aug 26, 2023

honggyukim Aug 26, 2023

honggyukim Aug 26, 2023

gichoel commented Aug 27, 2023 •

edited

Loading

honggyukim commented Aug 28, 2023

gichoel commented Aug 28, 2023

honggyukim commented Aug 29, 2023

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

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

Conversation

gichoel commented Aug 21, 2023

honggyukim commented Aug 22, 2023 • edited Loading

honggyukim left a comment

Choose a reason for hiding this comment

honggyukim Aug 24, 2023

Choose a reason for hiding this comment

gichoel commented Aug 25, 2023 • edited Loading

gichoel commented Aug 25, 2023

honggyukim left a comment

Choose a reason for hiding this comment

honggyukim Aug 26, 2023 • edited Loading

Choose a reason for hiding this comment

honggyukim Aug 26, 2023

Choose a reason for hiding this comment

honggyukim Aug 26, 2023

Choose a reason for hiding this comment

honggyukim Aug 26, 2023

Choose a reason for hiding this comment

honggyukim Aug 26, 2023

Choose a reason for hiding this comment

gichoel commented Aug 27, 2023 • edited Loading

honggyukim commented Aug 28, 2023

gichoel commented Aug 28, 2023

honggyukim commented Aug 29, 2023

honggyukim commented Aug 22, 2023 •

edited

Loading

gichoel commented Aug 25, 2023 •

edited

Loading

honggyukim Aug 26, 2023 •

edited

Loading

gichoel commented Aug 27, 2023 •

edited

Loading