Non-SFI Mode: Enable compiling exception_test for x86-32.

Non-SFI Mode: Enable compiling exception_test for x86-32.

Add --target to enable inline assembly with memory constraints, and remove naclret from assembly for non-SFI target.

Fix support for Non-SFI mode in AddBiasForPNaCl to set correct pnacl-bias.


TESTED= ./scons -j20 --mode=dbg-host,nacl bitcode=1 nonsfi_nacl=1 \
  pnacl_generate_pexe=0 platform=arm run_exception_test
  # compiles and crashes with SIGSEGV
TESTED= ./scons -j20 --mode=dbg-host,nacl bitcode=1 nonsfi_nacl=1 \
  pnacl_generate_pexe=0 platform=x86-32 run_exception_test
  # compiles and crashes with SIGSEGV

BUG= https://code.google.com/p/chromium/issues/detail?id=408879

Committed: http://src.chromium.org/viewvc/native_client?view=rev&revision=13824

[Junichi Uekawa, 2014-09-30 13:30:37]

uekawa@chromium.org changed reviewers:
+ hamaji@chromium.org, hidehiko@chromium.org, mseaborn@chromium.org

[Junichi Uekawa, 2014-09-30 13:30:38]

Hi,

Now it seems to be possible to compile exception_test.

I tried adding -target for site_tools/naclsdk.py:AddBiasForPNaCl, but it seems
to break tests/toolchain/eh_loop_break_noopt_noframe.nonfinal.pexe with a
message "error: undefined reference to 'operator new(unsigned long)'" on 
./scons --verbose -k -j20 --mode=nacl,opt-host platform=arm bitcode=1
skip_trusted_tests=1 do_not_run_tests=1 naclsdk_validate=0                      
                     

I've wondered if it should be somewhere else restricted to nonsfi-nacl, but I
guess this is causing some ABI difference somewhere.

[Junichi Uekawa, 2014-09-30 13:43:40]

On 2014/09/30 13:30:38, Junichi Uekawa wrote:
> Hi,
> 
> Now it seems to be possible to compile exception_test.
> 
> I tried adding -target for site_tools/naclsdk.py:AddBiasForPNaCl, but it seems
> to break tests/toolchain/eh_loop_break_noopt_noframe.nonfinal.pexe with a
> message "error: undefined reference to 'operator new(unsigned long)'" on 
> ./scons --verbose -k -j20 --mode=nacl,opt-host platform=arm bitcode=1
> skip_trusted_tests=1 do_not_run_tests=1 naclsdk_validate=0                    
 
>                      
> 
> I've wondered if it should be somewhere else restricted to nonsfi-nacl, but I
> guess this is causing some ABI difference somewhere.

Hmm.. maybe not.

I was wondering if it's possible to work around "=a" which is i386-specific, and
use some other constraint instead, 
or restrict passing the option in some way since it seems to have side effects.

[Mark Seaborn, 2014-09-30 20:32:30]

https://codereview.chromium.org/616813002/diff/60002/site_scons/site_tools/na...
File site_scons/site_tools/naclsdk.py (right):

https://codereview.chromium.org/616813002/diff/60002/site_scons/site_tools/na...
site_scons/site_tools/naclsdk.py:361: '--target=i686-unknown-nacl'],
This is fine to do, but if we're using the arch-specific "--target" on x86-32
here we should do it for the x86-64 and ARM cases too, otherwise the
inconsistency could cause confusion later.

If that turns out not to work for x86-64 and ARM, there are two fallbacks:

 * Set --target in AllowInlineAssembly().
 * Otherwise, set --target in tests/exception_test.

https://codereview.chromium.org/616813002/diff/60002/tests/exception_test/dir...
File tests/exception_test/direction_flag_x86.S (right):

https://codereview.chromium.org/616813002/diff/60002/tests/exception_test/dir...
tests/exception_test/direction_flag_x86.S:51: #if NONSFI_MODE
Can you make this "#if defined(__native_client_nonsfi__)" instead?

https://codereview.chromium.org/616813002/diff/60002/tests/exception_test/exc...
File tests/exception_test/exception_test.c (right):

https://codereview.chromium.org/616813002/diff/60002/tests/exception_test/exc...
tests/exception_test/exception_test.c:185: if (NONSFI_MODE) {
As with the previous change, can you only do changes for making this test
compile+link, please?

I'd like to leave reviewing runtime fixes to the later change when the test does
run successfully.

[Junichi Uekawa, 2014-10-01 04:17:13]

https://codereview.chromium.org/616813002/diff/60002/site_scons/site_tools/na...
File site_scons/site_tools/naclsdk.py (right):

https://codereview.chromium.org/616813002/diff/60002/site_scons/site_tools/na...
site_scons/site_tools/naclsdk.py:361: '--target=i686-unknown-nacl'],
On 2014/09/30 20:32:29, Mark Seaborn wrote:
> This is fine to do, but if we're using the arch-specific "--target" on x86-32
> here we should do it for the x86-64 and ARM cases too, otherwise the
> inconsistency could cause confusion later.
> 
> If that turns out not to work for x86-64 and ARM, there are two fallbacks:
> 
>  * Set --target in AllowInlineAssembly().
>  * Otherwise, set --target in tests/exception_test.

AddBiasForPNaCl is used elsewhere for using the defines only so it seems risky
to add --target here.

moved to AllowInlineAssembly().

https://codereview.chromium.org/616813002/diff/60002/tests/exception_test/dir...
File tests/exception_test/direction_flag_x86.S (right):

https://codereview.chromium.org/616813002/diff/60002/tests/exception_test/dir...
tests/exception_test/direction_flag_x86.S:51: #if NONSFI_MODE
On 2014/09/30 20:32:29, Mark Seaborn wrote:
> Can you make this "#if defined(__native_client_nonsfi__)" instead?

pacl-clang doesn't seem to pass -D __native_client_nonsfi__ to a .S file.

https://codereview.chromium.org/616813002/diff/60002/tests/exception_test/exc...
File tests/exception_test/exception_test.c (right):

https://codereview.chromium.org/616813002/diff/60002/tests/exception_test/exc...
tests/exception_test/exception_test.c:185: if (NONSFI_MODE) {
On 2014/09/30 20:32:29, Mark Seaborn wrote:
> As with the previous change, can you only do changes for making this test
> compile+link, please?
> 
> I'd like to leave reviewing runtime fixes to the later change when the test
does
> run successfully.

Done.

[Junichi Uekawa, 2014-10-01 06:37:21]

On 2014/10/01 04:17:13, Junichi Uekawa wrote:
>
https://codereview.chromium.org/616813002/diff/60002/site_scons/site_tools/na...
> File site_scons/site_tools/naclsdk.py (right):
> 
>
https://codereview.chromium.org/616813002/diff/60002/site_scons/site_tools/na...
> site_scons/site_tools/naclsdk.py:361: '--target=i686-unknown-nacl'],
> On 2014/09/30 20:32:29, Mark Seaborn wrote:
> > This is fine to do, but if we're using the arch-specific "--target" on
x86-32
> > here we should do it for the x86-64 and ARM cases too, otherwise the
> > inconsistency could cause confusion later.
> > 
> > If that turns out not to work for x86-64 and ARM, there are two fallbacks:
> > 
> >  * Set --target in AllowInlineAssembly().
> >  * Otherwise, set --target in tests/exception_test.
> 
> AddBiasForPNaCl is used elsewhere for using the defines only so it seems risky
> to add --target here.
> 
> moved to AllowInlineAssembly().
> 
>
https://codereview.chromium.org/616813002/diff/60002/tests/exception_test/dir...
> File tests/exception_test/direction_flag_x86.S (right):
> 
>
https://codereview.chromium.org/616813002/diff/60002/tests/exception_test/dir...
> tests/exception_test/direction_flag_x86.S:51: #if NONSFI_MODE
> On 2014/09/30 20:32:29, Mark Seaborn wrote:
> > Can you make this "#if defined(__native_client_nonsfi__)" instead?
> 
> pacl-clang doesn't seem to pass -D __native_client_nonsfi__ to a .S file.
> 
>
https://codereview.chromium.org/616813002/diff/60002/tests/exception_test/exc...
> File tests/exception_test/exception_test.c (right):
> 
>
https://codereview.chromium.org/616813002/diff/60002/tests/exception_test/exc...
> tests/exception_test/exception_test.c:185: if (NONSFI_MODE) {
> On 2014/09/30 20:32:29, Mark Seaborn wrote:
> > As with the previous change, can you only do changes for making this test
> > compile+link, please?
> > 
> > I'd like to leave reviewing runtime fixes to the later change when the test
> does
> > run successfully.
> 
> Done.

ptal

[Junichi Uekawa, 2014-10-01 07:56:03]

https://codereview.chromium.org/616813002/diff/120001/SConstruct
File SConstruct (right):

https://codereview.chromium.org/616813002/diff/120001/SConstruct#newcode3044
SConstruct:3044: '-mfloat-abi=hard'])
hmm.. It seems to be affecting existing ARM tests in ways that I don't quite
understand yet.

[Junichi Uekawa, 2014-10-01 07:58:58]

On 2014/10/01 07:56:03, Junichi Uekawa wrote:
> https://codereview.chromium.org/616813002/diff/120001/SConstruct
> File SConstruct (right):
> 
> https://codereview.chromium.org/616813002/diff/120001/SConstruct#newcode3044
> SConstruct:3044: '-mfloat-abi=hard'])
> hmm.. It seems to be affecting existing ARM tests in ways that I don't quite
> understand yet.

what's strange is that PS6 and PS8 shouldn't be different except for whitespace.

[Mark Seaborn, 2014-10-01 16:49:47]

On 2014/10/01 07:58:58, Junichi Uekawa wrote:
> On 2014/10/01 07:56:03, Junichi Uekawa wrote:
> > https://codereview.chromium.org/616813002/diff/120001/SConstruct
> > File SConstruct (right):
> > 
> > https://codereview.chromium.org/616813002/diff/120001/SConstruct#newcode3044
> > SConstruct:3044: '-mfloat-abi=hard'])
> > hmm.. It seems to be affecting existing ARM tests in ways that I don't quite
> > understand yet.
> 
> what's strange is that PS6 and PS8 shouldn't be different except for
whitespace.

Did you look at the trybot failures on patchset 8?

Most of them are failures in "gclient revert" -- infrastructure flakiness.  The
other failure, on the ARM hardware bot, is in run_minidump_crash_in_lib_test --
presumably a flaky test (or the ARM hardware bot being flaky).  So these are not
caused by your change.

[Mark Seaborn, 2014-10-01 17:02:30]

https://codereview.chromium.org/616813002/diff/60002/site_scons/site_tools/na...
File site_scons/site_tools/naclsdk.py (right):

https://codereview.chromium.org/616813002/diff/60002/site_scons/site_tools/na...
site_scons/site_tools/naclsdk.py:361: '--target=i686-unknown-nacl'],
On 2014/10/01 04:17:13, Junichi Uekawa wrote:
> On 2014/09/30 20:32:29, Mark Seaborn wrote:
> > This is fine to do, but if we're using the arch-specific "--target" on
x86-32
> > here we should do it for the x86-64 and ARM cases too, otherwise the
> > inconsistency could cause confusion later.
> > 
> > If that turns out not to work for x86-64 and ARM, there are two fallbacks:
> > 
> >  * Set --target in AllowInlineAssembly().
> >  * Otherwise, set --target in tests/exception_test.
> 
> AddBiasForPNaCl is used elsewhere for using the defines only so it seems risky
> to add --target here.
>
> moved to AllowInlineAssembly().

Oops, yes.  I misread this change and thought you were adding this to
PNaClForceNative() (which would be OK).  I agree that adding this to
AddBiasForPNaCl() isn't so good, since AddBiasForPNaCl() is only supposed to
affect #defines.

Moving this to AllowInlineAssembly() is the right thing to do...

[Mark Seaborn, 2014-10-01 17:04:00]

LGTM

https://codereview.chromium.org/616813002/diff/120001/tests/exception_test/na...
File tests/exception_test/nacl.scons (right):

https://codereview.chromium.org/616813002/diff/120001/tests/exception_test/na...
tests/exception_test/nacl.scons:11: env.Append(CPPDEFINES=[['NONSFI_MODE',
Maybe add a comment: We #define NONSFI_MODE here because
"__native_client_nonsfi__" currently does not get #defined for .S files in the
Scons build.

[Junichi Uekawa, 2014-10-02 02:33:11]

https://codereview.chromium.org/616813002/diff/120001/tests/exception_test/na...
File tests/exception_test/nacl.scons (right):

https://codereview.chromium.org/616813002/diff/120001/tests/exception_test/na...
tests/exception_test/nacl.scons:11: env.Append(CPPDEFINES=[['NONSFI_MODE',
On 2014/10/01 17:04:00, Mark Seaborn wrote:
> Maybe add a comment: We #define NONSFI_MODE here because
> "__native_client_nonsfi__" currently does not get #defined for .S files in the
> Scons build.

I found out why __native_client_nonsfi__ wasn't defined, it was
--pnacl-i686-bias when I really wanted was --pnacl-bias=x86-32-nonsfi.

I've included the fix and looks better now.

[Junichi Uekawa, 2014-10-02 04:35:04]

The CQ bit was checked by uekawa@chromium.org

[commit-bot: I haz the power, 2014-10-02 04:35:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/616813002/160001

[commit-bot: I haz the power, 2014-10-02 16:25:02]

Committed patchset #10 (id:160001) as 13824