First attempt to capture parser/translation errors in browser.

First attempt to capture parser/translation errors in browser.

Adds a notion of an (optional) error stream to the existing
log and emit streams. If not specified, the log stream is used.
Error messages in parser/translation are sent to this new error
stream.

In the browser compiler server, a separate error (string) stream is
created to capture errors. Method onEndCallBack returns the contents
of the error stream (if non-empty) instead of a generic error message.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=4138

[Karl, 2015-04-16 23:01:58]

kschimpf@google.com changed reviewers:
+ jvoung@chromium.org, stichnot@chromium.org

[jvoung (off chromium), 2015-04-17 18:15:10]

https://codereview.chromium.org/1052833003/diff/1/src/IceBrowserCompileServer...
File src/IceBrowserCompileServer.cpp (right):

https://codereview.chromium.org/1052833003/diff/1/src/IceBrowserCompileServer...
src/IceBrowserCompileServer.cpp:85: if (ErrorStream) {
ErrorStream *should* be non-null at this point, since it was able to check the
ErrorCode, and that wouldn't have been a real error code unless
startCompileThread() is already done (and that's where ErrorStream is created).

https://codereview.chromium.org/1052833003/diff/1/src/IceBrowserCompileServer...
src/IceBrowserCompileServer.cpp:90: return strdup(Message);
strdup is usually given the c_str() -- or am I missing something?

https://codereview.chromium.org/1052833003/diff/1/src/IceBrowserCompileServer...
src/IceBrowserCompileServer.cpp:154: ELFStream.get(), Flags.
ErrorStream.getStream()));
Should that be a comma instead of a period?

Might be better to group the streams and leave Flags at the end?

[Karl, 2015-04-17 21:33:52]

I also made all three streams be defined when calling constructor GlobalContext.

https://codereview.chromium.org/1052833003/diff/1/src/IceBrowserCompileServer...
File src/IceBrowserCompileServer.cpp (right):

https://codereview.chromium.org/1052833003/diff/1/src/IceBrowserCompileServer...
src/IceBrowserCompileServer.cpp:85: if (ErrorStream) {
On 2015/04/17 18:15:09, jvoung wrote:
> ErrorStream *should* be non-null at this point, since it was able to check the
> ErrorCode, and that wouldn't have been a real error code unless
> startCompileThread() is already done (and that's where ErrorStream is
created).

I agree that it shouldn't be null. I wasn't 100% sure so I added the test to be
safe. However, since you know the code more, I will remove the check.

https://codereview.chromium.org/1052833003/diff/1/src/IceBrowserCompileServer...
src/IceBrowserCompileServer.cpp:90: return strdup(Message);
On 2015/04/17 18:15:09, jvoung wrote:
> strdup is usually given the c_str() -- or am I missing something?

My bad. Fixing.

https://codereview.chromium.org/1052833003/diff/1/src/IceBrowserCompileServer...
src/IceBrowserCompileServer.cpp:154: ELFStream.get(), Flags.
ErrorStream.getStream()));
On 2015/04/17 18:15:09, jvoung wrote:
> Should that be a comma instead of a period?
> 
> Might be better to group the streams and leave Flags at the end?

Fixed, and moved the streams into a group.

[jvoung (off chromium), 2015-04-20 21:41:57]

Otherwise LGTM

https://codereview.chromium.org/1052833003/diff/60001/src/IceBrowserCompileSe...
File src/IceBrowserCompileServer.cpp (right):

https://codereview.chromium.org/1052833003/diff/60001/src/IceBrowserCompileSe...
src/IceBrowserCompileServer.cpp:86: return strdup(Message);
remove the dup'ed strdup: "return strdup(Message);"

https://codereview.chromium.org/1052833003/diff/60001/src/IceBrowserCompileSe...
File src/IceBrowserCompileServer.h (right):

https://codereview.chromium.org/1052833003/diff/60001/src/IceBrowserCompileSe...
src/IceBrowserCompileServer.h:81: std::string getContents() { return
StrBuf.str(); }
Might be able to return a reference to the string instead? Const also?

IceString?

[native-client-reviews_googlegroups.com, 2015-04-20 22:10:38]

Jan,

I guess I no longer understand Makefile.standalone. How do I build and test
the browser version of pnacl-sz?

When I compile it, I don't get any error messages in
IceBrowserCompilerServer.cpp, even when I put something explicitly wrong in
it.

I tried:
make -f Makefile.standalone PNACL_BROWSER_TRANSLATOR=1, but that clearly
doesn't work (this argument doesn't get copied to the compile line for
IceBrowserCompilerServer.cpp).

Thanks

Karl Schimpf

On Mon, Apr 20, 2015 at 2:41 PM, <jvoung@chromium.org> wrote:

> Otherwise LGTM
>
>
>
>
https://codereview.chromium.org/1052833003/diff/60001/src/IceBrowserCompileSe...
> File src/IceBrowserCompileServer.cpp (right):
>
>
>
https://codereview.chromium.org/1052833003/diff/60001/src/IceBrowserCompileSe...
> src/IceBrowserCompileServer.cpp:86: return strdup(Message);
> remove the dup'ed strdup: "return strdup(Message);"
>
>
>
https://codereview.chromium.org/1052833003/diff/60001/src/IceBrowserCompileSe...
> File src/IceBrowserCompileServer.h (right):
>
>
>
https://codereview.chromium.org/1052833003/diff/60001/src/IceBrowserCompileSe...
> src/IceBrowserCompileServer.h:81: std::string getContents() { return
> StrBuf.str(); }
> Might be able to return a reference to the string instead? Const also?
>
> IceString?
>
> https://codereview.chromium.org/1052833003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[jvoung (off chromium), 2015-04-20 22:49:47]

Okay, so that's why your compiler didn't catch some of these things.

I didn't make it compile the browser version by default (and didn't make the
Makefile do different things depending on "PNACL_BROWSER_TRANSLATOR=1" or not,
unlike the MINIMAL=0/1). It's instead a different target "sb". So you have to
run "make -f Makefile.standalone sb". That will only check that the code
compiles though.

This also requires having run "toolchain_build_pnacl.py --build-sbtc
sandboxed_translators" to fill the "work" directory with the sandboxed
libraries.




On 2015/04/20 22:10:38, native-client-reviews_googlegroups.com wrote:
> Jan,
> 
> I guess I no longer understand Makefile.standalone. How do I build and test
> the browser version of pnacl-sz?
> 
> When I compile it, I don't get any error messages in
> IceBrowserCompilerServer.cpp, even when I put something explicitly wrong in
> it.
> 
> I tried:
> make -f Makefile.standalone PNACL_BROWSER_TRANSLATOR=1, but that clearly
> doesn't work (this argument doesn't get copied to the compile line for
> IceBrowserCompilerServer.cpp).
> 
> Thanks
> 
> Karl Schimpf
> 
> On Mon, Apr 20, 2015 at 2:41 PM, <mailto:jvoung@chromium.org> wrote:
> 
> > Otherwise LGTM
> >
> >
> >
> >
>
https://codereview.chromium.org/1052833003/diff/60001/src/IceBrowserCompileSe...
> > File src/IceBrowserCompileServer.cpp (right):
> >
> >
> >
>
https://codereview.chromium.org/1052833003/diff/60001/src/IceBrowserCompileSe...
> > src/IceBrowserCompileServer.cpp:86: return strdup(Message);
> > remove the dup'ed strdup: "return strdup(Message);"
> >
> >
> >
>
https://codereview.chromium.org/1052833003/diff/60001/src/IceBrowserCompileSe...
> > File src/IceBrowserCompileServer.h (right):
> >
> >
> >
>
https://codereview.chromium.org/1052833003/diff/60001/src/IceBrowserCompileSe...
> > src/IceBrowserCompileServer.h:81: std::string getContents() { return
> > StrBuf.str(); }
> > Might be able to return a reference to the string instead? Const also?
> >
> > IceString?
> >
> > https://codereview.chromium.org/1052833003/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Native-Client-Reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:native-client-reviews+unsubscribe@googlegroups.com.
> To post to this group, send email to
mailto:native-client-reviews@googlegroups.com.
> Visit this group at http://groups.google.com/group/native-client-reviews.
> For more options, visit https://groups.google.com/d/optout.

[native-client-reviews_googlegroups.com, 2015-04-20 22:49:55]

Jan,

After talking to Derek, it appears that I need to use
toolchain_build_pnacl.py. Will fix syntax errors, run trybots, and submit
for re-review of IceBrowserCompilerServer.{h,cpp}.

Karl

Karl Schimpf

On Mon, Apr 20, 2015 at 3:10 PM, Karl Schimpf <kschimpf@google.com> wrote:

> Jan,
>
> I guess I no longer understand Makefile.standalone. How do I build and
> test the browser version of pnacl-sz?
>
> When I compile it, I don't get any error messages in
> IceBrowserCompilerServer.cpp, even when I put something explicitly wrong in
> it.
>
> I tried:
> make -f Makefile.standalone PNACL_BROWSER_TRANSLATOR=1, but that clearly
> doesn't work (this argument doesn't get copied to the compile line for
> IceBrowserCompilerServer.cpp).
>
> Thanks
>
> Karl Schimpf
>
> On Mon, Apr 20, 2015 at 2:41 PM, <jvoung@chromium.org> wrote:
>
>> Otherwise LGTM
>>
>>
>>
>>
https://codereview.chromium.org/1052833003/diff/60001/src/IceBrowserCompileSe...
>> File src/IceBrowserCompileServer.cpp (right):
>>
>>
>>
https://codereview.chromium.org/1052833003/diff/60001/src/IceBrowserCompileSe...
>> src/IceBrowserCompileServer.cpp:86: return strdup(Message);
>> remove the dup'ed strdup: "return strdup(Message);"
>>
>>
>>
https://codereview.chromium.org/1052833003/diff/60001/src/IceBrowserCompileSe...
>> File src/IceBrowserCompileServer.h (right):
>>
>>
>>
https://codereview.chromium.org/1052833003/diff/60001/src/IceBrowserCompileSe...
>> src/IceBrowserCompileServer.h:81: std::string getContents() { return
>> StrBuf.str(); }
>> Might be able to return a reference to the string instead? Const also?
>>
>> IceString?
>>
>> https://codereview.chromium.org/1052833003/
>>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[jvoung (off chromium), 2015-04-20 22:55:51]

Right, if you do "toolchain_build_pnacl.py --build-sbtc
sandboxed_translators --install ..."

It will also build the thing along with the needed libraries.

After you do that once, "make -f Makefile.standalone sb" will be a faster way to
at least compile the thing.

To test, you have to install via the toolchain_build_pnacl.py command though.
Once installed, you can try running some scons tests like:

"./scons use_sandboxed_translator=1 bitcode=1 use_sz=1 platform=x86-32
smoke_tests"

Add --verbose if you like to check things.. Also remember to clobber the
"scons-out/nacl-x86-32-pnacl-subzero-sbtc-pexe-clang" directory first.




On 2015/04/20 22:49:55, native-client-reviews_googlegroups.com wrote:
> Jan,
> 
> After talking to Derek, it appears that I need to use
> toolchain_build_pnacl.py. Will fix syntax errors, run trybots, and submit
> for re-review of IceBrowserCompilerServer.{h,cpp}.
> 
> Karl
> 
> Karl Schimpf
> 
> On Mon, Apr 20, 2015 at 3:10 PM, Karl Schimpf <mailto:kschimpf@google.com>
wrote:
> 
> > Jan,
> >
> > I guess I no longer understand Makefile.standalone. How do I build and
> > test the browser version of pnacl-sz?
> >
> > When I compile it, I don't get any error messages in
> > IceBrowserCompilerServer.cpp, even when I put something explicitly wrong in
> > it.
> >
> > I tried:
> > make -f Makefile.standalone PNACL_BROWSER_TRANSLATOR=1, but that clearly
> > doesn't work (this argument doesn't get copied to the compile line for
> > IceBrowserCompilerServer.cpp).
> >
> > Thanks
> >
> > Karl Schimpf
> >
> > On Mon, Apr 20, 2015 at 2:41 PM, <mailto:jvoung@chromium.org> wrote:
> >
> >> Otherwise LGTM
> >>
> >>
> >>
> >>
>
https://codereview.chromium.org/1052833003/diff/60001/src/IceBrowserCompileSe...
> >> File src/IceBrowserCompileServer.cpp (right):
> >>
> >>
> >>
>
https://codereview.chromium.org/1052833003/diff/60001/src/IceBrowserCompileSe...
> >> src/IceBrowserCompileServer.cpp:86: return strdup(Message);
> >> remove the dup'ed strdup: "return strdup(Message);"
> >>
> >>
> >>
>
https://codereview.chromium.org/1052833003/diff/60001/src/IceBrowserCompileSe...
> >> File src/IceBrowserCompileServer.h (right):
> >>
> >>
> >>
>
https://codereview.chromium.org/1052833003/diff/60001/src/IceBrowserCompileSe...
> >> src/IceBrowserCompileServer.h:81: std::string getContents() { return
> >> StrBuf.str(); }
> >> Might be able to return a reference to the string instead? Const also?
> >>
> >> IceString?
> >>
> >> https://codereview.chromium.org/1052833003/
> >>
> >
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Native-Client-Reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:native-client-reviews+unsubscribe@googlegroups.com.
> To post to this group, send email to
mailto:native-client-reviews@googlegroups.com.
> Visit this group at http://groups.google.com/group/native-client-reviews.
> For more options, visit https://groups.google.com/d/optout.

[Karl, 2015-04-22 22:23:50]

Committed as 2f67b929c15a30d5ed43b9929cac95a96fec0bbe

Closing...

https://codereview.chromium.org/1052833003/diff/60001/src/IceBrowserCompileSe...
File src/IceBrowserCompileServer.cpp (right):

https://codereview.chromium.org/1052833003/diff/60001/src/IceBrowserCompileSe...
src/IceBrowserCompileServer.cpp:86: return strdup(Message);
On 2015/04/20 21:41:57, jvoung wrote:
> remove the dup'ed strdup: "return strdup(Message);"

Done.

https://codereview.chromium.org/1052833003/diff/60001/src/IceBrowserCompileSe...
File src/IceBrowserCompileServer.h (right):

https://codereview.chromium.org/1052833003/diff/60001/src/IceBrowserCompileSe...
src/IceBrowserCompileServer.h:81: std::string getContents() { return
StrBuf.str(); }
On 2015/04/20 21:41:57, jvoung wrote:
> Might be able to return a reference to the string instead? Const also?
> 
> IceString?

Done.