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

logger: Use a function-level static var for the logger object #6689

Merged
merged 2 commits into from
Jan 10, 2019

Conversation

rgacogne
Copy link
Member

Short description

This PR includes the commit from #6625, replaces #6639. I don't like the macro trick at all but I don't see any other way to make sure that the Logger object is correctly initialized before any use.

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)

@tih
Copy link
Contributor

tih commented May 29, 2018

I tested this on NetBSD/amd64, and verified that the authoritative server and the recursor both work correctly with this modification. Thus, it solves the issue I reported in #6624.

I will also test on NetBSD/arm, and report further once that has been done.

@tih
Copy link
Contributor

tih commented May 31, 2018

The recursor still crashes in exactly the same way on NetBSD/arm, I'm afraid.

@rgacogne
Copy link
Member Author

I'm now wondering whether the switch to thread_local is causing this issue on NetBSD/arm..

@tih
Copy link
Contributor

tih commented May 31, 2018

Can I do something to test whether thread_local is working properly? Might be a platform specific bug in GCC 6.4, I suppose.

@tih
Copy link
Contributor

tih commented Jun 1, 2018

Just to see what would happen, I built the authoritative server on the Pi, from the same code base, including the gpgsql module. It's running (and logging) just as it should, happily letting me create and modify zones, which it then serves correctly. Securing zones on it works, too.

So the crash in the recursor, the first time recursorThread() tries to log something, may of course be a GCC bug in the thread_local handling, but it doesn't seem all that likely to me.

For reference, the backtrace looks like this:

Program terminated with signal SIGSEGV, Segmentation fault.
#0 0x7a5598c4 in std::string::append(std::string const&) ()
from /usr/lib/libstdc++.so.8
[Current thread is 1 (process 2)]
(gdb) bt
#0 0x7a5598c4 in std::string::append(std::string const&) ()
from /usr/lib/libstdc++.so.8
#1 0x088d8b74 in Logger::operator<< (s=..., this=0x8da7f6c <getLogger()::log>)
at logger.cc:166
#2 Logger::operator<< (this=0x8da7f6c <getLogger()::log>, s=)
at logger.cc:172
#3 0x08971ddc in recursorThread (n=, worker=)
at pdns_recursor.cc:3436
[...]
(gdb) up
#1 0x088d8b74 in Logger::operator<< (s=..., this=0x8da7f6c <getLogger()::log>)
at logger.cc:166
166 pt.d_output.append(s);
(gdb) print s
$1 = (const std::string &) @0x79effc4c: {static npos = 4294967295,
_M_dataplus = {<std::allocator> = {<__gnu_cxx::new_allocator> = {}, },
_M_p = 0x7a13555c "Done priming cache with root hints"}}

Copy link
Member

@chbruyand chbruyand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM although I don't like the macro trick either. Perhaps putting getLogger() in a namespace would reduce confusion ?

@rgacogne
Copy link
Member Author

rgacogne commented Jan 8, 2019

Perhaps putting getLogger() in a namespace would reduce confusion ?

I'm not sure I understand what you have in mind?

@rgacogne rgacogne merged commit 40df43f into PowerDNS:master Jan 10, 2019
@rgacogne rgacogne deleted the logger-thread-local-static branch January 10, 2019 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants