Typo on simulator-mips64.cc L5163?

[Aki A]

Hello V8 team,

I've just found a potential typo in /src/execution /mips64/simulator-mips64.cc in the current master branch.

I think the Line number 5163:

  int wt_modulo = wt % (sizeof(T) * 8);

Should be changed to:

  T wt_modulo = wt % (sizeof(T) * 8);

because other valuables in this method use T as a data type.
Also, a different file, simulator-mips.cc, in the same directory has the same logic around L4890, and it uses T.

This is a very small change, and keeping type int may work.
Should I still send a pull request to the V8 repo?

Best,

Aki

[Zhi An Ng]

On Sun, May 9, 2021 at 4:45 PM Aki A <akihiro...@gmail.com> wrote:

Hello V8 team,

I've just found a potential typo in /src/execution /mips64/simulator-mips64.cc in the current master branch.

I think the Line number 5163:

  int wt_modulo = wt % (sizeof(T) * 8);

Should be changed to:

  T wt_modulo = wt % (sizeof(T) * 8);

Looking at the code, I think it is fine, since wt_modulo will never bigger than (sizeof(T) * 8), and the largest value of sizeof(T) is 8 (t == int64_t), so wt_modulo will never exceed 64, which fits inside an int.

Similarly T wt_modulo will work too. I can't immediately spot any bugs here.

It is a bit inconsistent between simulator-mips and simulator-mips64, if you would like to upload the change, the maintainers of mips64 can decide if they want to fix this inconsistency.

 

because other valuables in this method use T as a data type.
Also, a different file, simulator-mips.cc, in the same directory has the same logic around L4890, and it uses T.

This is a very small change, and keeping type int may work.
Should I still send a pull request to the V8 repo?

Best,

Aki

--
--
v8-dev mailing list
v8-...@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+un...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/1b8ee9d8-c5ec-40f1-baa1-480e70f6d215n%40googlegroups.com.

--

Best,

Zhi An

[Akihiro Asahara]

Hello Zhi,

Thank you for your reply, and reviewing the code.

Yes. This code should work. It's just a consistency issue.

> if you would like to upload the change, the maintainers of mips64 can decide if they want to fix this inconsistency.

OK. Will do.

However, this is the first time to send a pull request to Google Git. Should I just follow this instructions? https://v8.dev/docs/contribute
Or, I'm more familiar with GitHub. Is that OK if I submit my pull request to the V8 mirror repository on Github (https://github.com/v8/v8)?

Best,

Aki

To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/CAP2LTJ1aci2xcaDsXZPrw4-tRUxFpDB%3DWd_9Fnn%2BiEgBYzDmPA%40mail.gmail.com.

[Zhi An Ng]

On Mon, May 10, 2021 at 4:09 PM Akihiro Asahara <akihiro...@gmail.com> wrote:

Hello Zhi,

Thank you for your reply, and reviewing the code.

Yes. This code should work. It's just a consistency issue.

> if you would like to upload the change, the maintainers of mips64 can decide if they want to fix this inconsistency.

OK. Will do.

However, this is the first time to send a pull request to Google Git. Should I just follow this instructions? https://v8.dev/docs/contribute
Or, I'm more familiar with GitHub. Is that OK if I submit my pull request to the V8 mirror repository on Github (https://github.com/v8/v8)?

I believe the V8 mirror on GitHub is only a mirror and doesn't take contributions. https://v8.dev/docs/contribute is the right place for instructions. https://v8.dev/docs/source-code#sending-code-for-reviewing has some more instructions for contribution (downloading code, uploading, etc).

 

To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/CAMWTQ7sC3913fgmx93oA4%2BAoFSfNVDBmjSeJh72%2BeH%3Dtbo%2BZ1A%40mail.gmail.com.

--

Best,

Zhi An

[Aki A]

Hello Zhi,

It's Sunday, and it's a good day for open source activities!
I've just uploaded a patch about this:
https://chromium-review.googlesource.com/c/v8/v8/+/2897323

It's a really small change, but this is the first time I submit something to the V8 repo.
I know you are busy, but please let me know if something is wrong.

Best,

Aki