Fix #1707 x86 build #1708
Fix #1707 x86 build #1708
Conversation
ae8b768 to
c183754
Compare
There was a problem hiding this comment.
Please rebase your changes on to the current master (after running git pull). And the commit message should have a proper prefix. 'fix' is not a proper one. It can be 'build' or 'arch/x86' or 'dynamic'. In addition, please consider adding the actual compiler error message to see what's going on here since it might depend on the compiler and its version. It's a convention to indent by two spaces those messages.
arch/x86_64/mcount-insn.c
Outdated
| @@ -476,7 +476,7 @@ static int check_instrumentable(struct mcount_disasm_engine *disasm, cs_insn *in | |||
| case CS_GRP_PRIVILEGE: | |||
| case CS_GRP_BRANCH_RELATIVE: | |||
| return INSTRUMENT_FAIL_NO_DETAIL; | |||
| default: | |||
| default:; | |||
There was a problem hiding this comment.
Please don't mix two changes in a commit. This change should be separate from the Github workflow. Also you'd better have break statement in the following line.
.github/workflows/pr-test.yml
Outdated
| arch: | ||
| - x86_64 | ||
| - aarch64 | ||
| - arm64 |
There was a problem hiding this comment.
Aren't aarch64 and arm64 the same?
|
copy that will fix it tonight thanks |
5be10a1 to
baddcf4
Compare
.github/workflows/pr-test.yml
Outdated
| matrix: | ||
| arch: | ||
| - x86_64 | ||
| - aarch64 |
There was a problem hiding this comment.
Actually I'm not sure if it's really needed to run the unittest on multiple archs.
There was a problem hiding this comment.
Could please check the actual result? It seems it didn't run the test on an ARM (64-bit) machine.
There was a problem hiding this comment.
will check it today, sorry for the inconvenience
arch/x86_64/mcount-insn.c
Outdated
| @@ -477,6 +477,7 @@ static int check_instrumentable(struct mcount_disasm_engine *disasm, cs_insn *in | |||
| case CS_GRP_BRANCH_RELATIVE: | |||
| return INSTRUMENT_FAIL_NO_DETAIL; | |||
| default: | |||
| break; | |||
There was a problem hiding this comment.
Please move it after the comment.
|
@namhyung you are right, the maybe need this: |
|
I'm ok to drop the github actions and just to fix the build problem. |
|
Done |
|
For this kind of change, it'd be nice if you could add the actual compiler warning message to the commit like in dae3c72. Note that you may want to indent them by 2 spaces to prevent accidental commenting out if it starts with the hash (#). Anyway I prefer having them indented (even without the hash) to indicate they are copied from elsewhere. |
|
of course, I will learn your commit and revise mine, thanks for the advice again~ |
$ make
CC FPIC arch/x86_64/mcount-insn.o
uftrace/arch/x86_64/mcount-insn.c:479:3: error: label at \
end of compound statement
default:
^~~~~~~
Fixed: namhyung#1707
Signed-off-by: Yi Hong <zouzou0208@gmail.com>
|
Hi @yihong0618, it was my mistake. Thanks for the fix! |
This PR fix build on x86_64 fix #1707