Re: [lmi] Forcing linking of the library modules (again)

[Vadim Zeitlin]

On Sat, 04 Oct 2014 13:20:57 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2014-08-15 20:47Z, Vadim Zeitlin wrote:
GC> [...]
GC> >  The comment added by the patch basically explains what is going on: when
GC> > building lmi_wx.exe using MSVC, the file progress_meter_wx.cpp, which is
GC> > part of the skeleton library, is simply not linked in at all because the
GC> > main program doesn't reference any of its symbols and Microsoft linker
GC> > completely discards all the object files which it considers to be 
unneeded.
GC> > This results in set_progress_meter_creator() being never called, as the
GC> > global "ensure_setup" variable in this file is never initialized because
GC> > it's simply not present in the final binary.
GC> 
GC> I'm willing to apply such a patch if you can include every translation
GC> unit that's likely to have this problem (see below).

 I hadn't thought to check this when submitting the initial patch because
my goal was to keep the changes as minimal as possible and so I
intentionally limited them to wx code only, as this is the only part of the
code base that is currently being built with MSVC. However now that I did
look at the other files, it turns out that they actually don't risk having
this problem in any case (please see below for explanations) and so we
don't need to do anything for them.


GC> Still, I have to ask: how can this linker not be considered broken?

 Personally I do consider it broken, but the fact is that it has always
behaved like this and I don't believe for a moment that this is going to
change any time soon, so we have to live with it irrespectively of my
opinion about whether it's correct or not.


GC> >  So would you be willing to apply the patch adding these macros to all the
GC> > affected files (once I find all of them...)?
GC> 
GC> To help you find all of them...this should suffice:
GC>   grep 'volatile bool\|ensure_setup' *.?pp

 The search finds the following files:

- {file_command,progress_meter,system_command}_wx.cpp: these are the files
  that genuinely need the workaround.

- {file_command,progress_meter,system_command}_c[gl]i.cpp: while these may
  look similar at a first glance, they do *not* require any workaround,
  because they are only included directly into the various executables and
  are never part of a library which is then linked into an executable,
  which is the only case in which MSVC linker bug manifests.

- alert_{cgi,cli,wx}.cpp: this one doesn't need the workaround even for the
  wx version, as it is directly linked into main_wx and wx_test targets and
  so can never be discarded.

- mc_enum_types_aux.cpp: the same initialization construction is used here,
  but it is safe because this file also contains several external functions
  which are indeed referenced from the other object files and so doesn't
  risk to be discarded neither.

- dbnames.cpp and numeric_io_test.cpp: "volatile bool" here are not at top
  level and so have no relationship to the issue.

- test_tools_test.cpp: "volatile bool" at top level, but is used just to
  define some constants and so is irrelevant as well.


 To summarize, only the files already covered by the original patch need
the "in situ" part of the workaround and, consequently, as all these files
are used in skeleton_objects only, only main_wx.cpp and main_wx_test.cpp
need the "ex situ" part of it.


GC> instead, I've cloned this code:
GC>   
http://svn.wxwidgets.org/viewvc/wx/wxWidgets/trunk/include/wx/link.h?view=co
GC> to provide lmi-specific macros in new header 'force_linking.hpp'.

 I don't know if the above changes your opinion about the need for these
macros and this header. If it does, we could simply remove it and use my
original patch. If it doesn't, please use the attached patch instead.

 But I'd like to notice that there is a small problem with the new
LMI_FORCE_LINKING_{IN,EX}_SITU() macros: they are inconsistent in their
handling of the subsequent semicolon as the former requires it, while the
latter does not. I would prefer if the latter one required the semicolon as
well, i.e. remove the trailing semicolon from the line 61 of
force_linking.hpp, as not terminating the macro definition with a semicolon
is the common rule and also because I made the attached patch with this
change in mind. But if you prefer to add it to its line 45, this would work
too -- however the patch would need to be updated to not use the redundant
semicolons after the macros then.


GC> Exactly where must wxFORCE_LINK_MODULE be written? Given that the
GC> issue is quite global, I'd rather write it in as few places as
GC> possible.

 As per above, it turns out that it's only needed in the two places where
it was added by the original patch.

 Regards,
VZ

link-fix.diff
Description: Text document