Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1493793002:
MIPS: Fixing CLANG compilation warnings (Closed)
|
Created:
5 years ago by ivica.bogosavljevic Modified:
5 years ago CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionMIPS: Fixing CLANG compilation warnings
Fixing warnings which cause compilation to fail when compiling
using CLANG for MIPS
BUG=
Committed: https://crrev.com/171fb5caa13b9c0b2110ba8d6fc7afb620d3f8ba
Cr-Commit-Position: refs/heads/master@{#32619}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Changing stub type to F4 and removing unnecessary casts #Patch Set 3 : Changing stub type to F4 and removing unnecessary casts (2) #
Total comments: 1
Messages
Total messages: 21 (8 generated)
https://codereview.chromium.org/1493793002/diff/1/src/mips64/cpu-mips64.cc File src/mips64/cpu-mips64.cc (right): https://codereview.chromium.org/1493793002/diff/1/src/mips64/cpu-mips64.cc#ne... src/mips64/cpu-mips64.cc:38: long res; // NOLINT(runtime/int) Do we need long at here? syscall returns with int. https://codereview.chromium.org/1493793002/diff/1/test/cctest/test-assembler-... File test/cctest/test-assembler-mips64.cc (right): https://codereview.chromium.org/1493793002/diff/1/test/cctest/test-assembler-... test/cctest/test-assembler-mips64.cc:4633: static_cast<int>(rt_value), 0, 0, 0)); If F2 function type (with int params) is used above then why we need uint64_t rs_value, rt_value function args insted of int in run_align(uint64_t rs_value, uint64_t rt_value, uint8_t bp)? If run_align would get int params then we don't need reinterpret_cast<>s. Would not that work? Or if 64 bit alignment must be tested then F4 function type should be used, I guess.
ilija.pavlovic@imgtec.com changed reviewers: + Ilija.Pavlovic@imgtec.com
lgtm
ivica.bogosavljevic@imgtec.com changed reviewers: - Ilija.Pavlovic@imgtec.com
https://codereview.chromium.org/1493793002/diff/1/src/mips64/cpu-mips64.cc File src/mips64/cpu-mips64.cc (right): https://codereview.chromium.org/1493793002/diff/1/src/mips64/cpu-mips64.cc#ne... src/mips64/cpu-mips64.cc:38: long res; // NOLINT(runtime/int) On 2015/12/02 18:49:07, balazs.kilvady wrote: > Do we need long at here? syscall returns with int. On MIPS64, if we leave int here than we cannot compile it under CLANG. If we set long, it compiles both under GCC and CLANG. I executed all the tests and I didn't see any new failures or something suspicious. The specification is ambiguous, I read that syscall can return long (e.g. http://man7.org/linux/man-pages/man2/syscall.2.html) or int (on my system man syscall) https://codereview.chromium.org/1493793002/diff/1/test/cctest/test-assembler-... File test/cctest/test-assembler-mips64.cc (right): https://codereview.chromium.org/1493793002/diff/1/test/cctest/test-assembler-... test/cctest/test-assembler-mips64.cc:4633: static_cast<int>(rt_value), 0, 0, 0)); On 2015/12/02 18:49:07, balazs.kilvady wrote: > If F2 function type (with int params) is used above then why we need uint64_t > rs_value, rt_value function args insted of int in run_align(uint64_t rs_value, > uint64_t rt_value, uint8_t bp)? If run_align would get int params then we don't > need reinterpret_cast<>s. Would not that work? Or if 64 bit alignment must be > tested then F4 function type should be used, I guess. Acknowledged.
lgtm
The CQ bit was checked by ivica.bogosavljevic@imgtec.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1493793002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1493793002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1493793002/diff/40001/src/mips64/cpu-mips64.cc File src/mips64/cpu-mips64.cc (right): https://codereview.chromium.org/1493793002/diff/40001/src/mips64/cpu-mips64.c... src/mips64/cpu-mips64.cc:40: res = syscall(__NR_cacheflush, start, size, ICACHE); I guess the return type of syscall may depend on the libc/glibc of the target and its header filess. So an auto res = syscall(__NR_cacheflush, start, size, ICACHE); solution should always work, I think.
The CQ bit was checked by ivica.bogosavljevic@imgtec.com
The patchset sent to the CQ was uploaded after l-g-t-m from Ilija.Pavlovic@imgtec.com Link to the patchset: https://codereview.chromium.org/1493793002/#ps40001 (title: "Changing stub type to F4 and removing unnecessary casts (2)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1493793002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1493793002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== MIPS: Fixing CLANG compilation warnings Fixing warnings which cause compilation to fail when compiling using CLANG for MIPS BUG= ========== to ========== MIPS: Fixing CLANG compilation warnings Fixing warnings which cause compilation to fail when compiling using CLANG for MIPS BUG= Committed: https://crrev.com/171fb5caa13b9c0b2110ba8d6fc7afb620d3f8ba Cr-Commit-Position: refs/heads/master@{#32619} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/171fb5caa13b9c0b2110ba8d6fc7afb620d3f8ba Cr-Commit-Position: refs/heads/master@{#32619} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
