Dear Jens, You wrote:
- Fixed a library initialization problem.
Why do you think so? The safety suggested by the library_init class is still nothing more than a sham. Best wishes, Chris
Hi Chris, On Fri, May 06, 2005 at 01:59:20PM +0200, Chris Dams wrote:
- Fixed a library initialization problem. Why do you think so? The safety suggested by the library_init class is still nothing more than a sham.
Well, like most people I didn't think, I just read the NEWS(-file) ;-) But seriously, I thought you and Richie solved that issue more or less? Regards, Jens
Dear Jens, On Mon, 9 May 2005, Jens Vollinga wrote:
But seriously, I thought you and Richie solved that issue more or less?
Well, I submitted a patch that solved the issue. This patch was only partly applied. Actually, only the code-beautifying part of it was applied and this change may, depending on the compiler, not even generate a different *.so file. The reason for this is twofold. Firstly, my patch breaks binary compatibility. Secondly, Richy was not certain whether he liked that my patch throws all references to flyweights out of the library and only keeps flyweights as numeric*'s and as ex-es. Of course it is quite possible to make a five-or-so-line class that emulates the behaviour of references and initializes in a safe way. On the other hand, that would probably still break binary compatibility. Or maybe not, as I showed in a private mail, but that results is some dirty hacking, that could possibly break things for some people. Since Richy is apparently still thinking about what would be best, I am not sure what is going to be done. Best wishes, Chris
Dear Chris, On Mon, 9 May 2005, Chris Dams wrote:
On Mon, 9 May 2005, Jens Vollinga wrote:
But seriously, I thought you and Richie solved that issue more or less?
Well, I submitted a patch that solved the issue. This patch was only partly applied. Actually, only the code-beautifying part of it was applied and this change may, depending on the compiler, not even generate a different *.so file.
The reason for this is twofold. Firstly, my patch breaks binary compatibility. Secondly, Richy was not certain whether he liked that my patch throws all references to flyweights out of the library and only keeps flyweights as numeric*'s and as ex-es. Of course it is quite possible to make a five-or-so-line class that emulates the behaviour of references and initializes in a safe way. On the other hand, that would probably still break binary compatibility. Or maybe not, as I showed in a private mail, but that results is some dirty hacking, that could possibly break things for some people.
Since Richy is apparently still thinking about what would be best, I am not sure what is going to be done.
Actually, I'm not. Sorry if I gave the impression. As you notice, this binary compatibilty issue makes your patch eligible for GiNaC 1.4.0 at best. If your patch solves a real problem, then we should reconsider it. But my impression was that it only solved a theoretical problem that no user of the library could possibly hit. Wrong? Regards -richy. -- Richard B. Kreckel <http://www.ginac.de/~kreckel/> k
Dear Richy, On Thu, 9 Jun 2005, Richard B. Kreckel wrote:
Since Richy is apparently still thinking about what would be best, I am not sure what is going to be done.
Actually, I'm not. Sorry if I gave the impression.
As you notice, this binary compatibilty issue makes your patch eligible for GiNaC 1.4.0 at best. If your patch solves a real problem, then we should reconsider it. But my impression was that it only solved a theoretical problem that no user of the library could possibly hit. Wrong?
If a user has in some *.C file a line like ex my_funny_class::my_funny_constant=(exp(1)/Pi).evalf(); would this not result in precisely the same problem as with the line ex integral::relative_integration_error = power(10,-8).evalf(); in integral.C? Best, Chris
On Thu, 2005-06-09 at 22:49 +0200, Richard B. Kreckel wrote:
As you notice, this binary compatibilty issue makes your patch eligible for GiNaC 1.4.0 at best. If your patch solves a real problem, then we should reconsider it. But my impression was that it only solved a theoretical problem that no user of the library could possibly hit. Wrong?
Hello? Anybody home? You have code in GiNaC that suggest protection against static initialization order problems that in fact does not do anything whatsoever. It should either be fixed or removed. Does anybody care about this? This is the second time that Richy leaves the discussion by not responding. I am getting more than a little irritated by this. I have no idea how Richy got the idea that my path only solves a "theoretical problem". Perhaps some other developer could continue this discussion, since it appears that it is impossible to discuss with Richy. Chris.
Hi Chris, I really like to have this initialization order problem solved! I read the the discussion mails between you and Richy again and had a look at your patch from Jan 8. But still, I have to ask some stupid questions: - your patch does COMPLETELY solve the problem? (just to be sure I got it right ;-)) - but it has to go in 1.4.0 (says Richy)? (from a first glance I don't see why this is necessary) Regards, Jens On Mon, Jun 27, 2005 at 01:21:42PM +0200, Chris Dams wrote:
On Thu, 2005-06-09 at 22:49 +0200, Richard B. Kreckel wrote:
As you notice, this binary compatibilty issue makes your patch eligible for GiNaC 1.4.0 at best. If your patch solves a real problem, then we should reconsider it. But my impression was that it only solved a theoretical problem that no user of the library could possibly hit. Wrong?
Hello? Anybody home? You have code in GiNaC that suggest protection against static initialization order problems that in fact does not do anything whatsoever. It should either be fixed or removed. Does anybody care about this?
This is the second time that Richy leaves the discussion by not responding. I am getting more than a little irritated by this.
I have no idea how Richy got the idea that my path only solves a "theoretical problem". Perhaps some other developer could continue this discussion, since it appears that it is impossible to discuss with Richy.
Dear Jens, On Tue, 2005-06-28 at 12:43 +0200, Jens Vollinga wrote:
I really like to have this initialization order problem solved! I read the the discussion mails between you and Richy again and had a look at your patch from Jan 8. But still, I have to ask some stupid questions:
That is the wrong patch. The right one was sent on Jan 20. It was not sent to the entire mailing list because it was rather big. However, Richy's reply to it is in the archives. It is http://thep.physik.uni- mainz.de/pipermail/ginac-devel/2005-January/000759.html . I have put the mail together with its attachment on my homepage: http://www.theorphys.science.ru.nl/people/dams/20jan.txt .
- your patch does COMPLETELY solve the problem? (just to be sure I got it right ;-))
Yes. I checked this by reintroducing the .evalf() in the definition of integral::relative_integration_error and observing that if my patch is applied this no longer leads to a segmentation fault when using the *.a version of the library. The *.so library never segfaulted at my place anyway.
- but it has to go in 1.4.0 (says Richy)? (from a first glance I don't see why this is necessary)
I also thought that this should go into 1.4.0 because it changes the interface. I.e., it removes the references to flyweights. E.g. it removes _num_120. However, no *.h file uses the refs to flyweights. Users could be using the flyweights in their code and then it would break. However, the comments in utils.h say that users should not do this. So, having looked at it again, I think that it is safe to put it in 1.3.2. I tested this by compiling a program under 1.3.1 and after that running it again with the patched version of the library installed. This works correctly. Best wishes, Chris
Hi Chris, On Tue, Jun 28, 2005 at 04:31:25PM +0200, Chris Dams wrote:
I also thought that this should go into 1.4.0 because it changes the interface. I.e., it removes the references to flyweights. E.g. it removes _num_120. However, no *.h file uses the refs to flyweights. Users could be using the flyweights in their code and then it would break. However, the comments in utils.h say that users should not do this. So, having looked at it again, I think that it is safe to put it in 1.3.2. I tested this by compiling a program under 1.3.1 and after that running it again with the patched version of the library installed. This works correctly.
why did the references have to go? I don't understand this, yet. In the Jan 20th mail you say it crashes otherwise. Do you have an explanation? It is not so important for me, if not. It just would be nice to understand that behavior. Did it also crash with references when the line const ex _ex30 = _num30; had been changed to const ex _ex30 = _ex30; (i.e. your Jan 8th patch with that additional change) ?? Regards, Jens
Dear Jens, On Tue, 28 Jun 2005, Jens Vollinga wrote:
why did the references have to go? I don't understand this, yet. In the Jan 20th mail you say it crashes otherwise. Do you have an explanation?
The point is that the library_init class should make sure that when the very first compilation module (corresponding to some arbitrary *.cpp file) is initialized all static variables are initialized. However, this very first compilation module does not set the references. This is because as the situation is in GiNaC 1.3.1 only the _num_whatever_p pointers get a value when this first module is initialized. Note also that these references cannot be set by linker because at that time the pointers do not yet point anywhere. Hence these references must get their value at the point when utils.cpp is initialized. It can be that some of GiNaCs functions are called before that time and these may attempt to use the references and then it crashes. The conclusion from the previous paragraph is that the references should be intialized in library_init::library_init. However, how does one initialize references? Things like (numeric*)(some_ref)=the_thing_the_ref_should_point_to of course do not work because it is not the reference that is casted to a numeric* but the numeric object itself and that does not make sense. One could wrap the reference in some class and use that as a handle to initialize the reference, but that will certainly (?) break binary compatibility and could theoretically not work the same way for all compilers. Therefore, I thought that throwing away the references was the best option.
Did it also crash with references when the line const ex _ex30 = _num30; had been changed to const ex _ex30 = _ex30; (i.e. your Jan 8th patch with that additional change) ??
I do not remember the results of all the debugging that I did back in January, but it should also crash since the initialization of the refrences is not changed in any way. Note however that the removal of the destructor of library_init that occurs in the patch of Jan 8th would certainly break binary compatibility. Therefore, the patch of Jan 20th should be used and the one of Jan 8th should be forgotten. Also, after ititializing the exes with the numerics instead of with themselves their refcount would be 2 and I think that is strange, even if it does not cause trouble. Best wishes, Chris
Hi Chris, thanks for all the patience and help. I applied your patches to CVS now. Looks like we could go for a 1.3.2 soon ... Thanks, Jens
Dear Chris, On Mon, 27 Jun 2005, Chris Dams wrote:
On Thu, 2005-06-09 at 22:49 +0200, Richard B. Kreckel wrote:
As you notice, this binary compatibilty issue makes your patch eligible for GiNaC 1.4.0 at best. If your patch solves a real problem, then we should reconsider it. But my impression was that it only solved a theoretical problem that no user of the library could possibly hit. Wrong?
Hello? Anybody home?
Not until yesterday, actually.
You have code in GiNaC that suggest protection against static initialization order problems that in fact does not do anything whatsoever. It should either be fixed or removed. Does anybody care about this?
Yes, I do. And, apparently, others do, too. :)
This is the second time that Richy leaves the discussion by not responding. I am getting more than a little irritated by this.
Oh, I disliked your first patch of January 8. However, removing the references is more than fine with me. I had actually intended to apply it unmodified to GiNaC-1.4, but I was lazy. I apologize for not having signalled to you my intent to apply your patch to HEAD.
I have no idea how Richy got the idea that my path only solves a "theoretical problem".
It is a theoretical problem in the sense that to our knowledge nobody has been bitten by it. This is quite different from the problem introduced in release 1.3.0 and finally solved by your patch in release 1.3.1. There, all statically linked programs in several Linux distros crashed upon startup. Anyways, your patch is fine and it was certainly correct to apply it. What we could argue about is whether it was necessary to apply it to the 1.3 branch. The problem is that the patch removes symbols from the library. You have to look at it from the point of view of a distro package maintainer or a system administrator. These folks do not care how meaningful a symbol is, or how bugfree a function is. All they care about is that the programs that depend on the library still work (a useful definition of "to work" being: they can be started). Now, any patch that removes a used symbol from the library without changing the soname breaks all depending programs (as ldd -r will tell you without actually invoking the apps). Users will then complain that the "compatible" library upgrade broke their code. For future reference, folks: We define a library branch (1.3.x, 1.4.x, etc.) by an interface version. Symbols shouldn't be removed by going from release x.y.n to x.y.n+1. We deliberately don't care about semantics, e.g. function return values may change by a patchlevel bump. Otherwise, it will hardly be possible to fix algorithmic bugs. If the urge to change the interface in an incompatible way becomes pressing, then, well, let's just change the minor version to y+1! It would be great if we could continue with this practice. It is an established strategy, altough it would be considered simplistic by some. Feel free to read the libtool texinfo documentation or [0] for a more complete (and certainly more confusing) treatise. Anyway: thanks, Chris, for your analysis and patch! Regards -richy. [0] <http://people.redhat.com/drepper/dsohowto.pdf> -- Richard B. Kreckel <http://www.ginac.de/~kreckel/>
Dear Richy, On Wed, 2005-07-13 at 00:42 +0200, Richard B. Kreckel wrote:
Anyways, your patch is fine and it was certainly correct to apply it. What we could argue about is whether it was necessary to apply it to the 1.3 branch.
I would not have minded if it was applied later. It is just that there is no observable difference between the intention to apply the patch and it being stored in the /dev/null archive. For increased confusion I took "user" in "a theoretical problem that no user of the library could possibly hit" to mean "programmer who uses GiNaC". Thanks anyway for the clarification! Best wishes, Chris
Dear Chris, I answered your mail already weeks ago, but due to some confusion with our mailing list it went to nirvana. Sorry. On Fri, 6 May 2005, Chris Dams wrote:
You wrote:
- Fixed a library initialization problem.
Why do you think so? The safety suggested by the library_init class is still nothing more than a sham.
This is an exaggeration. The patch below that went in for this release did solve a real initialization crash. I'm sure you'ld agree this alone warrants that NEWS entry. RCS file: /home/cvs/GiNaC/ginac/integral.cpp,v retrieving revision 1.2 retrieving revision 1.3 diff -u -r1.2 -r1.3 --- GiNaC/ginac/integral.cpp 2004/10/14 15:36:45 1.2 +++ GiNaC/ginac/integral.cpp 2005/01/07 21:01:53 1.3 @@ -203,7 +203,7 @@ } int integral::max_integration_level = 15; -ex integral::relative_integration_error = power(10,-8).evalf(); +ex integral::relative_integration_error = 1e-8; ex subsvalue(const ex & var, const ex & value, const ex & fun) { Regards -richy. -- Richard B. Kreckel <http://www.ginac.de/~kreckel/>
participants (3)
-
Chris Dams
-
Jens Vollinga
-
Richard B. Kreckel