bugGnash - The GNU Flash player - Bugs: bug #38217, Testsuite build fails with...

 
 

bug #38217: Testsuite build fails with undefined symbols with --enable-visibility

Submitter:  Sandro Santilli <strk>
Submitted:  Fri 01 Feb 2013 06:04:29 AM UTC
   
 
Category:  build Severity:  3 - Normal
Release:  None Status:  Fixed
Privacy:  Public Assigned to:  strk
Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sun 14 Jul 2013 07:00:54 PM UTC, comment #16: 

As the only author of all the extensions, I believe only the fileio one is actually used by anyone. Or at least it has been used in the past. There really isn't a need for an extensions API, as the API for each extension mimics the library the extension is supporting. Extension are merely a wrapper. Printing a warning is probably all that needs to be done.

Rob Savoye <rsavoye>
Group administrator
Sun 14 Jul 2013 06:46:24 PM UTC, comment #15: 

Good question. I guess if we had a clear-defined API then extensions authors would stick to that, and that API would be made visible. If we don't then, well, we should declare a build with --enable-visibility as not-functional with extension and notify the case explicitly at runtime, with something like a WARNING when extensions enabling is requested (and the testsuite should avoid running tests for extensions in that case).

Sandro Santilli <strk>
Group Member
Sun 14 Jul 2013 11:16:28 AM UTC, comment #14: 

Is --enable-visibility not fundamentally incompatible with --enable-extensions? Although we could make the unresolved symbols visible, the idea of the extension framework is that an extension author can access any symbol in the Gnash core, which would require them modifying Gnash core, or, as is more practicable, to build without visibility.

Bastiaan Jacques <bjacques>
Group Member
Sat 13 Jul 2013 04:21:19 PM UTC, comment #13: 

bug #36642 is one reason not to enable by default, if we fix that then I'm happy with it being a default.

Thanks for double checking :)

Sandro Santilli <strk>
Group Member
Fri 12 Jul 2013 10:21:04 PM UTC, comment #12: 

Can you think of any reason why hidden visibility should not be enabled by default, now that even the testsuite builds with it? I can think of only reasons in favour[1], and most especially having it by default would prevent developers from breaking it by accident.

[1] http://gcc.gnu.org/wiki/Visibility

Bastiaan Jacques <bjacques>
Group Member
Thu 11 Jul 2013 05:36:49 PM UTC, comment #11: 

DSOTEXPORT macro added, will make more symbols visible when testsuite is enabled, for a start, but it will be easy to switch to a different behavior if anyone would want to build with low visibility while still running a (forceably smaller) testsuite.

http://git.savannah.gnu.org/cgit/gnash.git/commit/?id=cd54b5ab5acd6bd357750f142059089eebe4664c

Sandro Santilli <strk>
Group Member
Tue 09 Jul 2013 11:12:15 AM UTC, comment #10: 

I added --disable-testsuite to speed up cross builds, it doesn't really have any other purpose for native builds other than dropping a few warnings from configure. I doubt anyone else uses it anyway, so I'm not sure this needs "fixing".

Rob Savoye <rsavoye>
Group administrator
Tue 09 Jul 2013 09:36:44 AM UTC, comment #9: 


> To make things simple and chunkable, what about starting with the TESTDSOEXPORT macro, currently defaulting to be an alias for DSOEXPORT ? We can discuss configure switches later


Doing that would also allow us to measure whether my concern is significant enough to merit making the extra exports conditional.

Bastiaan Jacques <bjacques>
Group Member
Tue 09 Jul 2013 09:28:55 AM UTC, comment #8: 

People that want to save the most space output could as well add --disable-testsuite. Those how don't will be able to run the testsuite, and thus will need those symbols.

If you prefer testsuite to be enabled by default that's a different topic.

I'm against an --enable-visibility=testsuite because you'll be then be back to "testsuite broken when adding --enable-visibility".

In any case the ./configure script should tell you if your configuration is non-functional (ie: requested testsuite but can't run due to visibility).

To make things simple and chunkable, what about starting with the TESTDSOEXPORT macro, currently defaulting to be an alias for DSOEXPORT ?  We can discuss configure switches later

Sandro Santilli <strk>
Group Member
Tue 09 Jul 2013 08:52:30 AM UTC, comment #7: 

I suspect few people actually build with --disable-testsuite. But I suppose --enable-visility could be given an =testsuite argument

Bastiaan Jacques <bjacques>
Group Member
Tue 09 Jul 2013 08:30:26 AM UTC, comment #6: 

How about having a TESTDSOEXPORT macro to only export when --disable-testsuite isn't given ?

Sandro Santilli <strk>
Group Member
Mon 08 Jul 2013 09:17:32 PM UTC, comment #5: 

Fixing this would require littering Gnash core with even more DSOEXPORTs than are presently in use--at a very casual count, 20 functions are to be marked. I'm inclined to say this should not be done, because it is reducing the effect of --enable-visibility, with the only benefit of letting developers run make check with this flag.

Having said that, it might be useful to add a warning to the output of configure that the testsuite won't work when this flag is used.

Bastiaan Jacques <bjacques>
Group Member
Fri 01 Feb 2013 08:58:50 AM UTC, comment #4: 

The only difference I see in dumpconfig output is that the broken build has '-fvisibility=hidden'.

It could really be that MovieTester lacks a DSOEXPORT...

Sandro Santilli <strk>
Group Member
Fri 01 Feb 2013 07:39:16 AM UTC, comment #3: 

with --enable-visibility the build still fails as of commit:118600cd59d4ebfc6b557b162cfe25e9f510da53

make[2]: Entering directory `/home/src/gnash/build/head/vanilla/testsuite/samples'
  CXX    clip_as_button2_TestRunner-clip_as_button2-TestRunner.o
  CXXLD  clip_as_button2-TestRunner
../../testsuite/.libs/libtestsuite.a(libtestsuite_la-MovieTester.o): In function `gnash::MovieTester::findDisplayItemByName(gna
sh::MovieClip const&, std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)':
/usr/src/gnash/build/head/vanilla/testsuite/../../../../gnash-head/testsuite/MovieTester.cpp:337: undefined reference to `gnash
::getStringTable(gnash::as_object const&)'


make dumpconfig extract:

 CXXFLAGS: -g -O2          -W     -Wall     -Wcast-qual     -Wpointer-arith     -Wreturn-type     -Wnon-virtual-dtor     -Wunused      -fvisibility-inlines-hidden -fvisibility=hidden -fPIE -fstack-protector --param ssp-buffer-size=4 -Wformat -Werror=format-security
 LDFLAGS: -Wl,--as-needed -fPIE -pie -Wl,-z,relro -Wl,-z,now

Configure line:

 configure --enable-visibility

Sandro Santilli <strk>
Group Member
Fri 01 Feb 2013 06:38:40 AM UTC, comment #2: 

I confirm it's the --enable-visibility flag breaking this.
Now trying with latest commit

Sandro Santilli <strk>
Group Member
Fri 01 Feb 2013 06:07:42 AM UTC, comment #1: 

I see commits of Rob about visibility and flags.
Will try again after pulling 118600cd59d4ebfc6b557b162cfe25e9f510da53

Sandro Santilli <strk>
Group Member
Fri 01 Feb 2013 06:04:29 AM UTC, original submission:  

While the core builds all fine, running "make check" fails to build some MovieTester based things in some kind of conditions.

I'm still debugging the conditions.
What I know is that a vanilla ./configure works fine.

I'm not sure which ./configure switches or FLAGS overriding break it. Still debugging. See bug #36642 for previous talk about this.

Sandro Santilli <strk>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by bjacques (Posted a comment)
  • -email is unavailable- added by strk (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2013-07-13 strk Open/ClosedOpen Closed
    2013-07-12 bjacques StatusReady For Test Fixed
    2013-07-11 strk StatusWont Fix Ready For Test
        Assigned toNone strk
    2013-07-08 bjacques StatusNone Wont Fix
    2013-02-01 strk SummaryTestsuite build fails with undefined symbols Testsuite build fails with undefined symbols with --enable-visibility

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code