Log4cxx
  1. Log4cxx
  2. LOGCXX-159

Initialization of local static objects out of order on Linux

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: Build
    • Labels:
      None
    • Environment:
      Linux (Redhat FC5), gcc-4.1.1, GNU ld 2.16, APR 1.2.2, APR-util 1.2.7

      Description

      (There is a bug that describes similar symptoms, LOGCXX-112, now closed; I couldn't figure out how to re-open it).

      I'm getting SEGFAULTs while trying to access log4cxx functions from a destructor of a global class instance. After some digging here's what I found: it seems that local static variables used by some log4cxx functions get destructed too early, (e.g., the string in NDC::getNull). The destructors of these object get called before the static destructors of user classes (I verified this in a debugger). I can also demonstrate this behavior in a completely isolated (not using log4cxx), simple program. I actually suspect this may be a "feature" of gcc and/or GNU ld, not necessarly a problem with log4cxx, since like I said it happens consitently with or without log4cxx code.

      I'm quite sure changing local static vars to be global (either standalone or as static class-members) will solve this.

      1. LOGCXX-159_standalone_test.tar.gz
        0.9 kB
        Davlet Panech
      2. LOGCXX-159_static_locals.patch
        21 kB
        Davlet Panech
      3. main.cpp
        0.4 kB
        Davlet Panech

        Issue Links

          Activity

          Hide
          Davlet Panech added a comment -

          A complete minimal program that demonstrates this problem.

          Show
          Davlet Panech added a comment - A complete minimal program that demonstrates this problem.
          Hide
          Davlet Panech added a comment -

          The attachments:

          main.cpp – a complete minimal program that demonstrates this problem
          LOG4CXX-159_standalone_test.tar.gz: a standalone app + .SO that shows the order of construction/destruction of various kinds of static objects (2 source files + Makefile).

          Show
          Davlet Panech added a comment - The attachments: main.cpp – a complete minimal program that demonstrates this problem LOG4CXX-159_standalone_test.tar.gz: a standalone app + .SO that shows the order of construction/destruction of various kinds of static objects (2 source files + Makefile).
          Hide
          Davlet Panech added a comment -

          Another thing: the solution described in LOGCXX-112 doesn't appear to be in the source tree.

          Show
          Davlet Panech added a comment - Another thing: the solution described in LOGCXX-112 doesn't appear to be in the source tree.
          Hide
          Curt Arnold added a comment -

          There was a substantial effort to eliminate non-local (aka global) static members from log4cxx due to an large number of seg faults on initialization that occurred with log4cxx 0.9.7. When you use non-local static members, there is no control over the order that non-static members in different translation units are initialized. For rationale, see item 47 in "Effective C++: Second Edition".

          Use of local static members (that is static variables within the scope of a function) eliminated all the reported bad initialization order segfaults, however there are still opportunities for an unfortunate order of destruction as you have apparently observed. For an log4cxx static to be guaranteed not be destructed before a particular destructor is called, the static must be accessed in the corresponding constructor or before. In the case of the NDC::getNull(), the correct response might be to eliminate the "optimization" of using a static. You should be able to work around the particular problem until the problem is resolved by forcing the initialization to occur before the constructor of your non-local static. For example:

          // access to NDC::getNull() to force it to be initialized
          NDC::getNull();

          // your static class that logs in its destructor
          FooBar MY_FOO_BAR;

          Show
          Curt Arnold added a comment - There was a substantial effort to eliminate non-local (aka global) static members from log4cxx due to an large number of seg faults on initialization that occurred with log4cxx 0.9.7. When you use non-local static members, there is no control over the order that non-static members in different translation units are initialized. For rationale, see item 47 in "Effective C++: Second Edition". Use of local static members (that is static variables within the scope of a function) eliminated all the reported bad initialization order segfaults, however there are still opportunities for an unfortunate order of destruction as you have apparently observed. For an log4cxx static to be guaranteed not be destructed before a particular destructor is called, the static must be accessed in the corresponding constructor or before. In the case of the NDC::getNull(), the correct response might be to eliminate the "optimization" of using a static. You should be able to work around the particular problem until the problem is resolved by forcing the initialization to occur before the constructor of your non-local static. For example: // access to NDC::getNull() to force it to be initialized NDC::getNull(); // your static class that logs in its destructor FooBar MY_FOO_BAR;
          Hide
          Davlet Panech added a comment -

          Okay, that sort of works, although there are other cases like this (e.g., Transcoder::encode), and hunting down all such instances is a little difficult.

          But anyway... I can think of a couple of ways of forcing the initialization of statics in well-defined order in the library itself, so that no user intervention is required. I would be willing to help, if you are interested.

          D.

          Show
          Davlet Panech added a comment - Okay, that sort of works, although there are other cases like this (e.g., Transcoder::encode), and hunting down all such instances is a little difficult. But anyway... I can think of a couple of ways of forcing the initialization of statics in well-defined order in the library itself, so that no user intervention is required. I would be willing to help, if you are interested. D.
          Hide
          Curt Arnold added a comment -

          My current though would be to add explicit calls to all functions that return local statics to Logger::getLogger. That would likely force construction of the log4cxx statics to occur before on in the constructor of the non-local statics in the the calling application which should result in them being destructed after the destructor of the non-local statics. Possibly many of the statics are just statics because they were so log4j and may not justify the added complexity in log4cxx where statics are more problematic.

          Show
          Curt Arnold added a comment - My current though would be to add explicit calls to all functions that return local statics to Logger::getLogger. That would likely force construction of the log4cxx statics to occur before on in the constructor of the non-local statics in the the calling application which should result in them being destructed after the destructor of the non-local statics. Possibly many of the statics are just statics because they were so log4j and may not justify the added complexity in log4cxx where statics are more problematic.
          Hide
          Davlet Panech added a comment -

          I think we can avoid making these dummy calls on every invocation of getLogger():

          • Create a separate "initialization" function that calls those other functions with local statics.
          • Protect it with a global integer reference count, so that it only does its thing on the first invocation
          • Make sure this function is called whenever any log4cxx header is included, i.e., force a static class instance – whose constructor calls our "initialization" function – into each of the user's translation units that #include any log4cxx headers.

          There's a whole bunch of local static constant std::string's in various places (e.g., DefaultConfigurator::getConfigurationFileName), maybe it would be a good idea to convert those to normal stack variables, like you mentioned. Otherwise it may get out of hand.

          Can I go ahead and try making these changes? I'll need commit access to your repository at some point. I'm not sure how your submission process works... let me know.

          Thanks,
          D.

          Show
          Davlet Panech added a comment - I think we can avoid making these dummy calls on every invocation of getLogger(): Create a separate "initialization" function that calls those other functions with local statics. Protect it with a global integer reference count, so that it only does its thing on the first invocation Make sure this function is called whenever any log4cxx header is included, i.e., force a static class instance – whose constructor calls our "initialization" function – into each of the user's translation units that #include any log4cxx headers. There's a whole bunch of local static constant std::string's in various places (e.g., DefaultConfigurator::getConfigurationFileName), maybe it would be a good idea to convert those to normal stack variables, like you mentioned. Otherwise it may get out of hand. Can I go ahead and try making these changes? I'll need commit access to your repository at some point. I'm not sure how your submission process works... let me know. Thanks, D.
          Hide
          Davlet Panech added a comment -

          Hmmm... come to think about it, there maybe another problem: are these static locals thread-safe? If two threads call the same function with a static local at about the same time, what happens? I don't think the compiler would mutex them automatically (maybe MSVC would, but UNIX compilers likely wouldn't).

          The only real solution then is to convert these vars to global pointers, and explicitely allocate/deallocate them (via "new") in the init/uninit function. And then, inject calls to init/uninit to user's translation units via a static initializer object (this last step could be conditional on some #define).

          D.

          Show
          Davlet Panech added a comment - Hmmm... come to think about it, there maybe another problem: are these static locals thread-safe? If two threads call the same function with a static local at about the same time, what happens? I don't think the compiler would mutex them automatically (maybe MSVC would, but UNIX compilers likely wouldn't). The only real solution then is to convert these vars to global pointers, and explicitely allocate/deallocate them (via "new") in the init/uninit function. And then, inject calls to init/uninit to user's translation units via a static initializer object (this last step could be conditional on some #define). D.
          Hide
          Curt Arnold added a comment -

          The potential threading issue on static initialization I don't believe is a real issue since initialization sequence would be triggered on module load which is a single threaded operation. Commit rights are granted by a vote after a history of patch submissions has been established and a contributors license agreement (http://www.apache.org/licenses) has been executed. I appreciate your enthusiasm. Unfortunately, I'm on deadline with a client and am not able to dig into the issue at the moment, but I should be able to look at it this weekend. If you'd like to discuss this further, we should move this to log4cxx-dev as this type of design discussion should go on there and not in JIRA,

          Show
          Curt Arnold added a comment - The potential threading issue on static initialization I don't believe is a real issue since initialization sequence would be triggered on module load which is a single threaded operation. Commit rights are granted by a vote after a history of patch submissions has been established and a contributors license agreement ( http://www.apache.org/licenses ) has been executed. I appreciate your enthusiasm. Unfortunately, I'm on deadline with a client and am not able to dig into the issue at the moment, but I should be able to look at it this weekend. If you'd like to discuss this further, we should move this to log4cxx-dev as this type of design discussion should go on there and not in JIRA,
          Hide
          Davlet Panech added a comment -

          LOGCXX-159_static_locals.patch – ensure static locals are constructed before any user code (incl. static ctors). NOTE: this patch creates 2 new files (include/log4cxx/helpers/staticinitializer.h and src/staticinitializer.cpp).

          Show
          Davlet Panech added a comment - LOGCXX-159 _static_locals.patch – ensure static locals are constructed before any user code (incl. static ctors). NOTE: this patch creates 2 new files (include/log4cxx/helpers/staticinitializer.h and src/staticinitializer.cpp).
          Hide
          Curt Arnold added a comment -

          The same program no longer seems to crash and the proposed remedy is troublesome. Closing issue since no further action seems likely.

          Show
          Curt Arnold added a comment - The same program no longer seems to crash and the proposed remedy is troublesome. Closing issue since no further action seems likely.

            People

            • Assignee:
              Curt Arnold
              Reporter:
              Davlet Panech
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development