[PATCH] PR58867 ASan and UBSan tests not run for installed testing.

[Maxim Ostapenko]

Hi,

some time ago, Andrew wrote a patch that fixes PR58867
(http://patchwork.ozlabs.org/patch/286866/), but for some reasons it
wasn't committed to trunk.

This is resurrected Andrew's patch, extended to support Tsan testsuite.

Tested on x86_64-pc-linux-gnu, ok to commit?

-Maxim

[Maxim Ostapenko]

Adding Joseph S. Myers as bug reporter.

Post by Maxim Ostapenko
Hi,
some time ago, Andrew wrote a patch that fixes PR58867
(http://patchwork.ozlabs.org/patch/286866/), but for some reasons it
wasn't committed to trunk.
This is resurrected Andrew's patch, extended to support Tsan testsuite.
Tested on x86_64-pc-linux-gnu, ok to commit?
-Maxim

[Mike Stump]

Hi,
some time ago, Andrew wrote a patch that fixes PR58867 (http://patchwork.ozlabs.org/patch/286866/), but for some reasons it
wasn't committed to trunk.
This is resurrected Andrew's patch, extended to support Tsan testsuite.
Tested on x86_64-pc-linux-gnu, ok to commit?

So, I’d like to hear from Andrew and/or Jakub if there are any objections… I glanced through the patch, and it seems reasonable enough. Let’s give em a chance to respond… If no objections, Ok.

[Jakub Jelinek]

Post by Mike Stump

Hi,
some time ago, Andrew wrote a patch that fixes PR58867 (http://patchwork.ozlabs.org/patch/286866/), but for some reasons it
wasn't committed to trunk.
This is resurrected Andrew's patch, extended to support Tsan testsuite.
Tested on x86_64-pc-linux-gnu, ok to commit?

So, I’d like to hear from Andrew and/or Jakub if there are any objections…
I glanced through the patch, and it seems reasonable enough. Let’s give
em a chance to respond… If no objections, Ok.

Ok with me. Seems I've reviewed the patch back in December 2013, but
Andrew has not posted an updated patch with the requested changes,
but apparently Maxim did incorporate those changes.

Jakub

[Marcus Shawcroft]

On 1 October 2014 08:09, Maxim Ostapenko

Post by Maxim Ostapenko
Hi,
some time ago, Andrew wrote a patch that fixes PR58867
(http://patchwork.ozlabs.org/patch/286866/), but for some reasons it
wasn't committed to trunk.
This is resurrected Andrew's patch, extended to support Tsan testsuite.
Tested on x86_64-pc-linux-gnu, ok to commit?
-Maxim

Hi,

This patch results in wide spread failures in our bare metal test
runs. I have not dug into the issue in detail but it appears that the
issue is related to the attempted use of set_ld_library_path_env_vars
and restore_ld_library_path_env_vars to save and restore nested
contexts.

Consider ubsan/ubsan.exp:

load_lib gcc-dg.exp
...
dg-init
ubsan_init
...
ubsan_finish

The gcc-dg.exp library load calls set_ld_library_path_env_vars the
first time an initializes GCC_EXEC_PREFIX.
The ubsan_init call makes a further call to
set_ld_library_path_env_vars which has no effect.
The ubsan_finish invokes restore_ld_library_path_env_vars, restoring
the environments as it was prior to the call to dg-init.
Any further test execution has now lost the original initialization of
GCC_EXEC_PREFIX by gcc-dg.exp

It seems to me that either this patch assumes that the
set/restore_ld_library_path_env foo is capable of maintaining a stack
of saved contexts... which it is not.

Prior to this patch ubsan_finish did not attempt to call
restore_ld_library_path_env_vars hence the gcc-dg.exp environment
remained in place for subsequent test executions.

Cheers
/Marcus

[Maxim Ostapenko]

Does it work without restore_ld_library_path in {asan, ubsan, tsan}_finish?

I see two opportunities to fix the issue:

1) Implement a stack of saved contexts.

2) Implement new functions, say {asan, ubsan,
tsan}_restore_ld_library_path, to be able {asan, ubsan, tsan}_finish
functions restore context correctly.

What solution is preferable?

-Maxim

Post by Marcus Shawcroft
On 1 October 2014 08:09, Maxim Ostapenko

Post by Maxim Ostapenko
Hi,
some time ago, Andrew wrote a patch that fixes PR58867
(http://patchwork.ozlabs.org/patch/286866/), but for some reasons it
wasn't committed to trunk.
This is resurrected Andrew's patch, extended to support Tsan testsuite.
Tested on x86_64-pc-linux-gnu, ok to commit?
-Maxim

Hi,
This patch results in wide spread failures in our bare metal test
runs. I have not dug into the issue in detail but it appears that the
issue is related to the attempted use of set_ld_library_path_env_vars
and restore_ld_library_path_env_vars to save and restore nested
contexts.
load_lib gcc-dg.exp
...
dg-init
ubsan_init
...
ubsan_finish
The gcc-dg.exp library load calls set_ld_library_path_env_vars the
first time an initializes GCC_EXEC_PREFIX.
The ubsan_init call makes a further call to
set_ld_library_path_env_vars which has no effect.
The ubsan_finish invokes restore_ld_library_path_env_vars, restoring
the environments as it was prior to the call to dg-init.
Any further test execution has now lost the original initialization of
GCC_EXEC_PREFIX by gcc-dg.exp
It seems to me that either this patch assumes that the
set/restore_ld_library_path_env foo is capable of maintaining a stack
of saved contexts... which it is not.
Prior to this patch ubsan_finish did not attempt to call
restore_ld_library_path_env_vars hence the gcc-dg.exp environment
remained in place for subsequent test executions.
Cheers
/Marcus

[Marcus Shawcroft]

On 8 October 2014 11:10, Maxim Ostapenko

Post by Maxim Ostapenko
Does it work without restore_ld_library_path in {asan, ubsan, tsan}_finish?
1) Implement a stack of saved contexts.
2) Implement new functions, say {asan, ubsan, tsan}_restore_ld_library_path,
to be able {asan, ubsan, tsan}_finish functions restore context correctly.
What solution is preferable?

Option 1 has the advantage that it places all of the context save and
restore in one place rather than spreading it around the
infrastructure.

Please can we revert this patch while a correct implementation is being worked?

Cheers
/Marcus

[Maxim Ostapenko]

Hm, as I see, others testsuites such as gfortran.exp, go.exp etc. do not
call restore_ld_library_path at all. Perhaps we could simply follow this
way?

Would failing tests still fail if remove restore_ld_library_path from
{asan, tsan, ubsan}_finish?

Post by Marcus Shawcroft
On 8 October 2014 11:10, Maxim Ostapenko

Post by Maxim Ostapenko
Does it work without restore_ld_library_path in {asan, ubsan, tsan}_finish?
1) Implement a stack of saved contexts.
2) Implement new functions, say {asan, ubsan, tsan}_restore_ld_library_path,
to be able {asan, ubsan, tsan}_finish functions restore context correctly.
What solution is preferable?

Option 1 has the advantage that it places all of the context save and
restore in one place rather than spreading it around the
infrastructure.
Please can we revert this patch while a correct implementation is being worked?
Cheers
/Marcus

[Jiong Wang]

Post by Maxim Ostapenko
Hm, as I see, others testsuites such as gfortran.exp, go.exp etc. do not
call restore_ld_library_path at all. Perhaps we could simply follow this
way?
Would failing tests still fail if remove restore_ld_library_path from
{asan, tsan, ubsan}_finish?

Hi Maxim,

verified those fails gone away on check-gcc.

but not sure whether this remove will cause problem on other test
environments which they are aimed to solve.

-- Jiong

Post by Maxim Ostapenko

Post by Marcus Shawcroft
On 8 October 2014 11:10, Maxim Ostapenko

Post by Maxim Ostapenko
Does it work without restore_ld_library_path in {asan, ubsan, tsan}_finish?
1) Implement a stack of saved contexts.
2) Implement new functions, say {asan, ubsan, tsan}_restore_ld_library_path,
to be able {asan, ubsan, tsan}_finish functions restore context correctly.
What solution is preferable?

Option 1 has the advantage that it places all of the context save and
restore in one place rather than spreading it around the
infrastructure.
Please can we revert this patch while a correct implementation is being worked?
Cheers
/Marcus

[Maxim Ostapenko]

Hi Jiong,

I couldn't reproduce tests failures on my box, perhaps I missed
something. How can I test this?

-Maxim

Post by Jiong Wang

Post by Maxim Ostapenko
Hm, as I see, others testsuites such as gfortran.exp, go.exp etc. do not
call restore_ld_library_path at all. Perhaps we could simply follow this
way?
Would failing tests still fail if remove restore_ld_library_path from
{asan, tsan, ubsan}_finish?

Hi Maxim,
verified those fails gone away on check-gcc.
but not sure whether this remove will cause problem on other test
environments which they are aimed to solve.
-- Jiong

Post by Maxim Ostapenko

Post by Marcus Shawcroft
On 8 October 2014 11:10, Maxim Ostapenko

Post by Maxim Ostapenko
Does it work without restore_ld_library_path in {asan, ubsan, tsan}_finish?
1) Implement a stack of saved contexts.
2) Implement new functions, say {asan, ubsan,
tsan}_restore_ld_library_path,
to be able {asan, ubsan, tsan}_finish functions restore context correctly.
What solution is preferable?

Option 1 has the advantage that it places all of the context save and
restore in one place rather than spreading it around the
infrastructure.
Please can we revert this patch while a correct implementation is being worked?
Cheers
/Marcus

[Jiong Wang]

Post by Maxim Ostapenko
Hi Jiong,
I couldn't reproduce tests failures on my box, perhaps I missed
something. How can I test this?

Hi Maxim,

What I mean is after this patch, those fails gone away on my test environment also.

I am just not sure whether the remove will cause it work on your and my test environment but fail on others. because looks like
these restore_ld_library_path is added deliberately.

Regards,
Jiong

Post by Maxim Ostapenko
-Maxim

Post by Jiong Wang

Post by Maxim Ostapenko
Hm, as I see, others testsuites such as gfortran.exp, go.exp etc. do not
call restore_ld_library_path at all. Perhaps we could simply follow this
way?
Would failing tests still fail if remove restore_ld_library_path from
{asan, tsan, ubsan}_finish?

Hi Maxim,
verified those fails gone away on check-gcc.
but not sure whether this remove will cause problem on other test
environments which they are aimed to solve.
-- Jiong

Post by Maxim Ostapenko

Post by Marcus Shawcroft
On 8 October 2014 11:10, Maxim Ostapenko

Post by Maxim Ostapenko
Does it work without restore_ld_library_path in {asan, ubsan, tsan}_finish?
1) Implement a stack of saved contexts.
2) Implement new functions, say {asan, ubsan,
tsan}_restore_ld_library_path,
to be able {asan, ubsan, tsan}_finish functions restore context correctly.
What solution is preferable?

Option 1 has the advantage that it places all of the context save and
restore in one place rather than spreading it around the
infrastructure.
Please can we revert this patch while a correct implementation is being worked?
Cheers
/Marcus