Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11016003:
[MIPS] Use break instruction as NACL_HALT for MIPS. (Closed)
|
Created:
8 years, 2 months ago by petarj Modified:
8 years, 2 months ago CC:
native-client-reviews_googlegroups.com Base URL:
http://src.chromium.org/native_client/trunk/src/native_client/ Visibility:
Public. |
Description[MIPS] Use break instruction as NACL_HALT for MIPS.
Replacing previous NACL_HALT (jr $zero) with a break instruction.
The commit queue is not working for NaCl at the moment, so we are
committing with:
NOTRY=true
However, we have run a try job manually, and it passed fine.
BUG= http://code.google.com/p/nativeclient/issues/detail?id=2275
TEST= pnacl/build.sh misc-tools
Committed: http://src.chromium.org/viewvc/native_client?view=rev&revision=9928
Patch Set 1 #
Total comments: 2
Patch Set 2 : Remove extra lines. #Messages
Total messages: 27 (0 generated)
Resubmitting the patch from http://codereview.chromium.org/10978045/ with a correct base-url.
+jfb, who works on the ARM validator and will look at the validator change. LGTM for the service_runtime parts. http://codereview.chromium.org/11016003/diff/1/src/trusted/service_runtime/ar... File src/trusted/service_runtime/arch/mips/sel_ldr_mips.h (right): http://codereview.chromium.org/11016003/diff/1/src/trusted/service_runtime/ar... src/trusted/service_runtime/arch/mips/sel_ldr_mips.h:10: #if !defined(__ASSEMBLER__) Nit: conditional #includes don't normally have empty lines around them, so: #if !defined(__ASSEMBLER__) # include "native_client/src/include/portability.h" #endif
Extra lines removed. A new patch set has been uploaded. http://codereview.chromium.org/11016003/diff/1/src/trusted/service_runtime/ar... File src/trusted/service_runtime/arch/mips/sel_ldr_mips.h (right): http://codereview.chromium.org/11016003/diff/1/src/trusted/service_runtime/ar... src/trusted/service_runtime/arch/mips/sel_ldr_mips.h:10: #if !defined(__ASSEMBLER__) On 2012/10/01 22:48:23, Mark Seaborn wrote: > Nit: conditional #includes don't normally have empty lines around them, so: > > #if !defined(__ASSEMBLER__) > # include "native_client/src/include/portability.h" > #endif Done.
I though having jumps in delay slots was unpredictable, shouldn't back-to-back halts fail to validate? The rest looks good, although I'd strongly advise you to rebase to the current ARM validator implementation: a lot has changed in it. There's probably a way that we could share code between ARM and MIPS, at least for the table parser side of things and some of the basic classes.
On 2012/10/02 23:20:55, JF wrote: > I though having jumps in delay slots was unpredictable, Yes, it is. > shouldn't back-to-back halts fail to validate? Can you please elaborate this? > The rest looks good, although I'd strongly advise you to rebase to the current ARM validator implementation: a lot has changed in it. There's probably a way that we could share code between ARM and MIPS, at least for the table parser side of things and some of the basic classes. Yes, should be doing that soon.
> > shouldn't back-to-back halts fail to validate? > > Can you please elaborate this? Padding code locations with a bunch of jr to zero should be flagged as undefined behavior (or really, any back-to-back old-style NACL_HALT), and on ARM we refuse to pass validation on anything that's undefined.
On 2012/10/03 00:00:21, JF wrote: > Padding code locations with a bunch of jr to zero should be flagged as undefined behavior (or really, any back-to-back old-style NACL_HALT), and on ARM we refuse to pass validation on anything that's undefined. We are not padding any location with a bunch of jr to zero. We are padding it with break instructions. What code is undefined?
> We are not padding any location with a bunch of jr to zero. We are padding it > with break instructions. > What code is undefined? I mean: in the old code that you're removing, the padding was jr 0, and if there's 2 or more such padding instructions then it should have been flagged as undefined. The reason I'm concerned is that it wasn't getting flagged as such, so an indirect branch to these pad instructions, even if aligned, would encounter undefined behavior because it would have two consecutive branches. I would expect that to get flagged as undefined and to fail validation, and want to make sure that users can't craft nexes with a branch within a delay slot.
On 2012/10/03 01:13:41, JF wrote: Validator already checks for these patterns and will fail for validation. You can not encounter this case anymore without validator signaling for it.
> Validator already checks for these patterns and will fail for validation. > You can not encounter this case anymore without validator signaling for it. Is there a test for this?
On 2012/10/03 01:26:36, JF wrote: > > Validator already checks for these patterns and will fail for validation. > > You can not encounter this case anymore without validator signaling for it. > > Is there a test for this? No, there is no test for it yet. In the previous comment, I said validator would fail when two consecutive jr are found - so, it would be triggered by previous NACL_HALT/NACL_HALT sequence. However, I realized that consecutive branches are not detected, so we need a new two-instruction pattern to add to MIPS validator, and make sure to flag it as undesired. Thanks for this. I will be uploading a change and a test for it hopefully tomorrow. It will be a separate CL.
On 2012/10/02 23:20:55, JF wrote: > The rest looks good Based on that, LGTM :-)
CQ is trying da patch. Follow status at https://nativeclient-status.appspot.com/cq/petarj@mips.com/11016003/4002
The commit queue went berserk retrying too often for a seemingly flaky test. Builder is nacl-arm_opt_panda, revision is 9912, job name was 11016003-4002 (previous was lost) (previous was lost) (previous was lost) (previous was lost).
CQ is trying da patch. Follow status at https://nativeclient-status.appspot.com/cq/petarj@mips.com/11016003/4002
On 4 October 2012 11:36, <commit-bot@chromium.org> wrote: > CQ is trying da patch. Follow status at > https://nativeclient-status.appspot.com/cq/petarj@mips.com/11016003/4002 > The commit queue is still broken for NaCl. As before, please kick off try jobs manually and when all the results come back, add the following to the commit message and re-tick the commit box: The commit queue is not working for NaCl at the moment, so we are committing with: NOTRY=true However, we have run a try job manually, and it passed fine. Since this change includes validator_mips changes, and I think validator_mips is built on platforms other than MIPS, a try run is needed for this change. Cheers, Mark -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To post to this group, send email to native-client-reviews@googlegroups.com. To unsubscribe from this group, send email to native-client-reviews+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/native-client-reviews?hl=en.
The commit queue went berserk retrying too often for a seemingly flaky test. Builder is nacl-arm_opt_panda, revision is 9925, job name was 11016003-4002 (previous was lost) (previous was lost) (previous was lost) (previous was lost).
Results from "git try". All tries succeeded. http://build.chromium.org/p/tryserver.nacl/builders/nacl-mac10.7_newlib_opt/b... http://build.chromium.org/p/tryserver.nacl/builders/nacl-mac10.7_glibc_opt/bu... http://build.chromium.org/p/tryserver.nacl/builders/nacl-mac10.6_glibc_opt/bu... http://build.chromium.org/p/tryserver.nacl/builders/nacl-mac10.6-newlib-dbg-c... http://build.chromium.org/p/tryserver.nacl/builders/nacl-mac10.6_newlib_opt/b... http://build.chromium.org/p/tryserver.nacl/builders/nacl-lucid32_newlib_dbg/b... http://build.chromium.org/p/tryserver.nacl/builders/nacl-lucid32_newlib_opt/b... http://build.chromium.org/p/tryserver.nacl/builders/nacl-lucid32_glibc_opt/bu... http://build.chromium.org/p/tryserver.nacl/builders/nacl-win64_newlib_dbg/bui... http://build.chromium.org/p/tryserver.nacl/builders/nacl-win32_glibc_opt/buil... http://build.chromium.org/p/tryserver.nacl/builders/nacl-win32_newlib_opt/bui... http://build.chromium.org/p/tryserver.nacl/builders/nacl-precise_64-newlib-x8... http://build.chromium.org/p/tryserver.nacl/builders/nacl-win64_newlib_opt/bui... http://build.chromium.org/p/tryserver.nacl/builders/nacl-win64_glibc_opt/buil...
http://build.chromium.org/p/tryserver.nacl/builders/nacl-mac10.7_newlib_opt/b... http://build.chromium.org/p/tryserver.nacl/builders/nacl-mac10.7_glibc_opt/bu... http://build.chromium.org/p/tryserver.nacl/builders/nacl-mac10.6_glibc_opt/bu... http://build.chromium.org/p/tryserver.nacl/builders/nacl-mac10.6-newlib-dbg-c... http://build.chromium.org/p/tryserver.nacl/builders/nacl-mac10.6_newlib_opt/b... http://build.chromium.org/p/tryserver.nacl/builders/nacl-lucid32_newlib_dbg/b... http://build.chromium.org/p/tryserver.nacl/builders/nacl-lucid32_newlib_opt/b... http://build.chromium.org/p/tryserver.nacl/builders/nacl-lucid32_glibc_opt/bu... http://build.chromium.org/p/tryserver.nacl/builders/nacl-win64_newlib_dbg/bui... http://build.chromium.org/p/tryserver.nacl/builders/nacl-win32_glibc_opt/buil... http://build.chromium.org/p/tryserver.nacl/builders/nacl-win32_newlib_opt/bui... http://build.chromium.org/p/tryserver.nacl/builders/nacl-precise_64-newlib-x8... http://build.chromium.org/p/tryserver.nacl/builders/nacl-win64_newlib_opt/bui... http://build.chromium.org/p/tryserver.nacl/builders/nacl-win64_glibc_opt/buil... http://build.chromium.org/p/tryserver.nacl/builders/nacl-lucid_64-newlib-x86_... http://build.chromium.org/p/tryserver.nacl/builders/nacl-lucid64_newlib_dbg/b... http://build.chromium.org/p/tryserver.nacl/builders/nacl-lucid64_glibc_opt/bu... http://build.chromium.org/p/tryserver.nacl/builders/nacl-lucid_64-newlib-x86_... http://build.chromium.org/p/tryserver.nacl/builders/nacl-lucid_64-newlib-arm_... http://build.chromium.org/p/tryserver.nacl/builders/nacl-lucid64_newlib_opt/b... http://build.chromium.org/p/tryserver.nacl/builders/nacl-lucid_64-newlib-x86_... http://build.chromium.org/p/tryserver.nacl/builders/nacl-lucid_64-newlib-dbg-... http://build.chromium.org/p/tryserver.nacl/builders/nacl-lucid64_newlib_dbg_v... http://build.chromium.org/p/tryserver.nacl/builders/nacl-lucid_64-newlib-x86_... http://build.chromium.org/p/tryserver.nacl/builders/nacl-lucid_64-newlib-arm_... http://build.chromium.org/p/tryserver.nacl/builders/nacl-lucid64_glibc_dbg_va... http://build.chromium.org/p/tryserver.nacl/builders/nacl-arm_opt_panda/builds... http://build.chromium.org/p/tryserver.nacl/builders/nacl-arm_hw_opt_panda/bui... http://build.chromium.org/p/tryserver.nacl/builders/nacl-lucid64_validator_op...
CQ is trying da patch. Follow status at https://nativeclient-status.appspot.com/cq/petarj@mips.com/11016003/4002
Change committed as 9928
On 4 October 2012 14:45, <petarj@mips.com> wrote: > Results from "git try". All tries succeeded. > Those are not all of the results -- you've only listed 14. NaCl's default trybot run currently includes 28 trybots. It's rather a lot, I know. The only definitive way to check for them is to look at the resulting number of e-mails you receive in the e-mail thread from the try server. Unfortunately, if the default set changes, there's no way to know how many to check for. The trybots can be slow because they are a shared resource, so receiving 14 successful results doesn't mean that all the trybots have passed. Some of the try jobs get queued so that they run after others. Cheers, Mark > http://build.chromium.org/p/**tryserver.nacl/builders/nacl-** > mac10.7_newlib_opt/builds/3057<http://build.chromium.org/p/tryserver.nacl/builders/nacl-mac10.7_newlib_opt/builds/3057> > http://build.chromium.org/p/**tryserver.nacl/builders/nacl-** > mac10.7_glibc_opt/builds/3047<http://build.chromium.org/p/tryserver.nacl/builders/nacl-mac10.7_glibc_opt/builds/3047> > http://build.chromium.org/p/**tryserver.nacl/builders/nacl-** > mac10.6_glibc_opt/builds/3043<http://build.chromium.org/p/tryserver.nacl/builders/nacl-mac10.6_glibc_opt/builds/3043> > http://build.chromium.org/p/**tryserver.nacl/builders/nacl-** > mac10.6-newlib-dbg-clang/**builds/3081<http://build.chromium.org/p/tryserver.nacl/builders/nacl-mac10.6-newlib-dbg-clang/builds/3081> > http://build.chromium.org/p/**tryserver.nacl/builders/nacl-** > mac10.6_newlib_opt/builds/3039<http://build.chromium.org/p/tryserver.nacl/builders/nacl-mac10.6_newlib_opt/builds/3039> > http://build.chromium.org/p/**tryserver.nacl/builders/nacl-** > lucid32_newlib_dbg/builds/3046<http://build.chromium.org/p/tryserver.nacl/builders/nacl-lucid32_newlib_dbg/builds/3046> > http://build.chromium.org/p/**tryserver.nacl/builders/nacl-** > lucid32_newlib_opt/builds/3043<http://build.chromium.org/p/tryserver.nacl/builders/nacl-lucid32_newlib_opt/builds/3043> > http://build.chromium.org/p/**tryserver.nacl/builders/nacl-** > lucid32_glibc_opt/builds/3054<http://build.chromium.org/p/tryserver.nacl/builders/nacl-lucid32_glibc_opt/builds/3054> > http://build.chromium.org/p/**tryserver.nacl/builders/nacl-** > win64_newlib_dbg/builds/3229<http://build.chromium.org/p/tryserver.nacl/builders/nacl-win64_newlib_dbg/builds/3229> > http://build.chromium.org/p/**tryserver.nacl/builders/nacl-** > win32_glibc_opt/builds/3253<http://build.chromium.org/p/tryserver.nacl/builders/nacl-win32_glibc_opt/builds/3253> > http://build.chromium.org/p/**tryserver.nacl/builders/nacl-** > win32_newlib_opt/builds/3286<http://build.chromium.org/p/tryserver.nacl/builders/nacl-win32_newlib_opt/builds/3286> > http://build.chromium.org/p/**tryserver.nacl/builders/nacl-** > precise_64-newlib-x86_64-**pnacl/builds/920<http://build.chromium.org/p/tryserver.nacl/builders/nacl-precise_64-newlib-x86_64-pnacl/builds/920> > http://build.chromium.org/p/**tryserver.nacl/builders/nacl-** > win64_newlib_opt/builds/3232<http://build.chromium.org/p/tryserver.nacl/builders/nacl-win64_newlib_opt/builds/3232> > http://build.chromium.org/p/**tryserver.nacl/builders/nacl-** > win64_glibc_opt/builds/3213<http://build.chromium.org/p/tryserver.nacl/builders/nacl-win64_glibc_opt/builds/3213> > > http://codereview.chromium.**org/11016003/<http://codereview.chromium.org/110... > -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To post to this group, send email to native-client-reviews@googlegroups.com. To unsubscribe from this group, send email to native-client-reviews+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/native-client-reviews?hl=en.
On 2012/10/04 21:53:09, Mark Seaborn wrote: > On 4 October 2012 14:45, <mailto:petarj@mips.com> wrote: > > > Results from "git try". All tries succeeded. > > > > Those are not all of the results -- you've only listed 14. NaCl's default > trybot run currently includes 28 trybots. It's rather a lot, I know. The > only definitive way to check for them is to look at the resulting number of > e-mails you receive in the e-mail thread from the try server. > Unfortunately, if the default set changes, there's no way to know how many > to check for. > > The trybots can be slow because they are a shared resource, so receiving 14 > successful results doesn't mean that all the trybots have passed. Some of > the try jobs get queued so that they run after others. > > Cheers, > Mark I guess you are replying from an email and have not looked at the commit page itself. It lists all 28 trybots (actually 29, 28 + "Triggerable arm_opt_panda_hw_tests on nacl-arm_hw_opt_panda"). The first message cut the list to 14 for some reason, but I have pasted all of them in the next message (which was not posted again to everyone). Take a look at the page itself. Regards, Petar
When I said "the commit page" I meant the code review page. I.e. http://codereview.chromium.org/11016003/
On 4 October 2012 15:24, <petarj@mips.com> wrote: > When I said "the commit page" I meant the code review page. > > I.e. http://codereview.chromium.org/11016003/ > Hmm, well, I don't see any trybot results listed on that page. And based on "svn log svn://svn.chromium.org/chrome-try/try-nacl", you haven't sent any try jobs recently that would show up on a Rietveld page, because none of the recent entries have an "issue=" field. You need to do "git checkout <branch> && git try" for the try job to show up on the Rietveld page for <branch>. Cheers, Mark -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To post to this group, send email to native-client-reviews@googlegroups.com. To unsubscribe from this group, send email to native-client-reviews+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/native-client-reviews?hl=en.
On 2012/10/04 22:29:22, Mark Seaborn wrote: > Hmm, well, I don't see any trybot results listed on that page. Just click "Expand All Messages" and you will see the links to the results in one of the messages. Regards, Petar
On 4 October 2012 15:32, <petarj@mips.com> wrote: > On 2012/10/04 22:29:22, Mark Seaborn wrote: > > Hmm, well, I don't see any trybot results listed on that page. >> > > Just click "Expand All Messages" and you will see the links to the results > in > one of the messages. > Ah, I see. Sorry for the confusion. In general, though, please can you try to get "git try" results to appear on Rietveld normally? It makes my task as reviewer easier if I can see the trybot results (hopefully green ones!) displayed. If that is not working, please let me know what commands you are running to kick off try jobs. Cheers, Mark -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To post to this group, send email to native-client-reviews@googlegroups.com. To unsubscribe from this group, send email to native-client-reviews+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/native-client-reviews?hl=en. |
