Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src/test/runtests.sh: drop 'readelf'-based tests #292

Closed
wants to merge 1 commit into from
Closed

src/test/runtests.sh: drop 'readelf'-based tests #292

wants to merge 1 commit into from

Conversation

trofi
Copy link
Contributor

@trofi trofi commented Feb 17, 2019

The 'readelf'-based tests cover a few situations:

  1. undefined symbols in shared libraries
  2. unexpected exports in shared libraries

Bug #575958 shows that [2.] implementation is too simplistic
in assuming that presence of relocation equals to export presence.

It is incorrect for PLT stubs and local symbols.
Let's just drop these tests.

If one neds to cover [1.] it is better to use LDFLAGS=-Wl,--no-undefined.

Reported-by: Benda Xu
Bug: https://bugs.gentoo.org/575958

The 'readelf'-based tests cover a few situations:
1. undefined symbols in shared libraries
2. unexpected exports in shared libraries

Bug #575958 shows that [2.] implementation is too simplistic
in assuming that presence of relocation equals to export presence.

It is incorrect for PLT stubs and local symbols.
Let's just drop these tests.

If one neds to cover [1.] it is better to use LDFLAGS=-Wl,--no-undefined.

Reported-by: Benda Xu
Bug: https://bugs.gentoo.org/575958
Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org>
@vapier
Copy link
Member

vapier commented Feb 17, 2019

i don't think we should remove the tests. these continue to catch mistakes with the API that never really goes away.

the test code already has a filter to try and remove "safe" relocs -- look at the parisc-specific filter.

@trofi
Copy link
Contributor Author

trofi commented Feb 17, 2019

You think all tests should stay?

https://bugs.gentoo.org/575958#c8 has bits of output for failed test. I'm not sure you can just filter out R_PPC64_REL24 and still protect against external symbols.

I'd say tests against *.so files directly are fine and could be tweaked time to time.

My main concern is interpreting readelf against *.o files:

readelf -Wr $(grep -l '#include[[:space:]]"librc\.h"' ${librc_srcdir}/*.c | sed 's:\.c$:.o:')

That will need to be extended to distinct LOCAL and HIDDEN symbols to be useful.

@williamh
Copy link
Contributor

@trofi I am open to removing these tests. If we come up with something better, we can always add new tests later. Also, it looks like if we drop those tests, we can remove the *.list files from the test directory.

@gyakovlev
Copy link
Contributor

if merged this will probably unblock #265

I did not look close enough what's really happening with readelf tests on FreeBSD, but output differs
https://github.com/OpenRC/openrc/pull/265/checks?check_run_id=58018033

@williamh
Copy link
Contributor

I had a pretty detailed IRC conversation with @trofi over the weekend and it looks like the best option for now is to remove these tests.

@williamh williamh closed this in b054aca Feb 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants