On Mi, 2010-01-27 at 11:03 +0000, Lukas Zeller wrote:
> I'm about to review merge the patches from the moblin/master branch
> into our libsynthesis repo - thanks to your earlier cleanup of the
> history it's a really straightforward task, thanks for that!
>
> The only change that I'm not sure about is:
>
> 71aef21545 (TDebugLoggerBase::DebugOpenBlock(): unused va_list)
>
> Doesn't the "static" prefix make that argument a global scope
> variable, and thus make the routine non-reentrant and non-threadsafe?
The variable is not used at all because the format string is NULL.
Previously the code simply passed NULL instead of a valid va_list, but
that didn't work on the Alpha architecture where va_list is not a
pointer.
After adding an uninitialized va_list, at some pointer GCC started
complaining about the "uninitialized" part. That's what the latest patch
is trying to solve.
> Just because in another commit, you point out that a va_list can't be
> re-used, which IMHO implies it is changed when it is processed by
> vsprintf. Of course, in this case with the list being empty we can be
> reasonably safe it is not changed or a change would not have any
> effect, even if multiple threads should try it simultaneously. Still,
> is that clean enough?
Because it is never written, I think this is safe.
> Second - I don't really see why declaring it static would mean
> anything for initialisation (apart from the fact that gcc seems not to
> be able to figure out it should complain about it then) - C-ish stuff
> is not implicitly initialized, neither in global scope nor on the
> stack.
In global scope it is filled with zero. Only on the stack is the content
undefined.
The following has the same effect:
static int a = 0;
static int b;
The only difference is in the implementation. The explicitly initialized
"a" is in the section of constants that gets loaded just like program
code. "b" is in a section that is cleared in memory when loading the
program and thus does not consume space in the binary.
> So my proposal would be just to call
> DebugOpenBlock(blockname,title,collapsed,fmt,...) with fmt=NULL and no
> optional arguments instead of DebugVOpenBlock:
>
> // open new Block without attribute list
> void TDebugLoggerBase::DebugOpenBlock(cAppCharP aBlockName, cAppCharP
> aBlockTitle, bool aCollapsed)
> {
> // we need a format and debug not completely off
> if (getMask() && aBlockName) {
> DebugOpenBlock(aBlockName,aBlockTitle,aCollapsed,NULL);
> }
> } // TDebugLoggerBase::DebugOpenBlock
>
> This is completely safe and we don't have to mess with the fact that
> apparently there's no proper way to initialize an empty va_list.
Yes, that looks like a cleaner solution, with slightly higher overhead.
--
Best Regards, Patrick Ohly
The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.
_______________________________________________
os-libsynthesis mailing list
os-libsynthesis@synthesis.ch
http://lists.synthesis.ch/mailman/listinfo/os-libsynthesis
