Re: Wrong indentation which leads to segfault in sc/source/ui/docshell/docfunc.cxx
On Tuesday 01 of January 2013, John LeMoyne Castle wrote: Julien, Looks to me like a very nice catch ... ... The next version (2011.07.05) http://opengrok.libreoffice.org/xref/core/sc/source/ui/docshell/docfunc.cxx ?r=2b88f6d32f572792597ccbb15276b9db52db7d10 is a commit labelled quot;change remaining manual loops to ScMarkData::iterator quot; and the braces disappear on just one of the loops. Because the braces were once there and, especially because the else block has ~ same action within the braces, I say that they should definitely come back. Even though, I am not at all clear what this code is doing (whilst autoformatting selected cells in Calc)... this is an obvious oopsie within a greater code cleanup that should be corrected. ... I note that, for all its type mushiness, BASIC will not allow this kind of mistake. The then-clause must either be all on one line (can do IF test THEN StmtA : StmtB : StmtC) and a multi-line then-clause must be terminated with End If or Else. http://tinderbox.libreoffice.org/cgi-bin/gunzip.cgi?tree=MASTERfull-log=1356992414.21678#20458 : 20458 In file included from stdin:1: 20459 /home/tinderbox/clang-master-build/sc/source/ui/docshell/docfunc.cxx:3743:17: warning: statement aligned as second statement in for body but not in a statement block [loplugin] 20460 SetWidthOrHeight( false,1,nRows, *itr, SC_SIZE_VISOPT, 0, false, false); 20461 ^ 20462 /home/tinderbox/clang-master-build/sc/source/ui/docshell/docfunc.cxx:3742:17: note: for body statement is here [loplugin] 20463 SetWidthOrHeight( sal_True, 1,nCols, *itr, SC_SIZE_VISOPT, STD_EXTRA_WIDTH, false, sal_True); 20464 ^ 20465 1 warning generated. -- Lubos Lunak l.lu...@suse.cz ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: Wrong indentation which leads to segfault in sc/source/ui/docshell/docfunc.cxx
Thank you John for your investigation about all this. In fact, I was leafing through some cppcheck reports. To simplify, I pushed some slight fixes in sc/source/core/tool/autoform.cxx (see http://cgit.freedesktop.org/libreoffice/core/commit/?id=2e1abe1c59b1121ffb5d46afe82ce985cb70c4db). So now we've got this: [sc/source/core/tool/autoform.cxx:1102] - [sc/source/core/tool/autoform.cxx:1105]: (performance) Variable 'bRet' is reassigned a value before the old one has been used. 1102 bRet = (rStream.GetError() == 0); 1103 //--- 1104 rStream (sal_uInt16)(maData.size() - 1); 1105 bRet = (rStream.GetError() == 0); 1106 MapType::iterator it = maData.begin(), itEnd = maData.end(); 1107 for (++it; bRet it != itEnd; ++it) // Skip the first item. 1108 bRet = it-second-Save(rStream, fileVersion); So bRet value of 1102 isn't taken into account since there's another assignation line 1105. But just after, I noticed that at line 1107 it variable was increased without checking it != itEnd So I checked bugs concerning autoformats and ran into fdo#47466. I gave a try to it by adding an if (it != itEnd) before line 1107 and finally had the bt of my first message. So I looked at the git history as you indicated and found about the missing braces was due to a slight mistake during a cleaning. I pushed a fix (see http://cgit.freedesktop.org/libreoffice/core/commit/?id=f843850ee9994653673ef5591aae875d7fb22fed). Now there are still the cppcheck report + the potential lacking of iterator test before loop Julien -- View this message in context: http://nabble.documentfoundation.org/Wrong-indentation-which-leads-to-segfault-in-sc-source-ui-docshell-docfunc-cxx-tp4026726p4026790.html Sent from the Dev mailing list archive at Nabble.com. ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: Wrong indentation which leads to segfault in sc/source/ui/docshell/docfunc.cxx
BTW, it's not cppcheck which had safe iterator since it just allow to scan sources. I rather think it's --enable-dbgutil in autogen.lastrun which triggered safe iterator. Julien -- View this message in context: http://nabble.documentfoundation.org/Wrong-indentation-which-leads-to-segfault-in-sc-source-ui-docshell-docfunc-cxx-tp4026726p4026829.html Sent from the Dev mailing list archive at Nabble.com. ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Wrong indentation which leads to segfault in sc/source/ui/docshell/docfunc.cxx
Hello,
1)
Trying to reproduce fdo#47466 with master sources updated this morning, I
had this:
#0 0x7ff8cd039475 in *__GI_raise (sig=optimized out) at
../nptl/sysdeps/unix/sysv/linux/raise.c:64
#1 0x7ff8cd03c6f0 in *__GI_abort () at abort.c:92
#2 0x7ff8cd8de31d in __gnu_debug::_Error_formatter::_M_error() const ()
from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#3 0x7ff8ae7c3f24 in
__gnu_debug::_Safe_iteratorstd::_Rb_tree_const_iteratorlt;short,
std::__debug::setshort, std::lesslt;short, std::allocatorshort
::operator* (
this=0x7fff96484bf0) at /usr/include/c++/4.7/debug/safe_iterator.h:261
#4 0x7ff8aee0bd21 in ScDocFunc::AutoFormat (this=0x288bd00, rRange=...,
pTabMark=0x297b6d0, nFormatNo=1, bRecord=true, bApi=false)
at
/home/julien/compile-libreoffice/libo/sc/source/ui/docshell/docfunc.cxx:3743
#5 0x7ff8af222bd0 in ScViewFunc::AutoFormat (this=0x296a7c0,
nFormatNo=1, bRecord=1 '\001') at
/home/julien/compile-libreoffice/libo/sc/source/ui/view/viewfun2.cxx:1553
#6 0x7ff8af102636 in ScCellShell::Execute (this=0x29a45a0, rReq=...) at
/home/julien/compile-libreoffice/libo/sc/source/ui/view/cellsh3.cxx:843
...
on console:
Objects involved in the operation:
iterator this @ 0x0x7fff96484bf0 {
type =
N11__gnu_debug14_Safe_iteratorISt23_Rb_tree_const_iteratorIsENSt7__debug3setIsSt4lessIsESaIsE
(mutable iterator);
state = past-the-end;
references sequence with type `NSt7__debug3setIsSt4lessIsESaIsEEE' @
0x0x7fff96484bf0
}
I noticed this:
3740 ScMarkData::iterator itr = aMark.begin(), itrEnd =
aMark.end();
3741 for (; itr != itrEnd *itr nTabCount; ++itr)
3742 SetWidthOrHeight( sal_True, 1,nCols, *itr,
SC_SIZE_VISOPT, STD_EXTRA_WIDTH, false, sal_True);
3743 SetWidthOrHeight( false,1,nRows, *itr,
SC_SIZE_VISOPT, 0, false, false);
3744 rDocShell.PostPaint( 0,0,*itr, MAXCOL,MAXROW,*itr,
3745 PAINT_GRID | PAINT_LEFT | PAINT_TOP
);
Should the for loop include all the block (until line 3745) or something
should be done?
2) It could be linked too
cppcheck detected this:
[sc/source/core/tool/autoform.cxx:749] -
[sc/source/core/tool/autoform.cxx:752]: (performance) Variable 'bRet' is
reassigned a value before the old one has been used.
[sc/source/core/tool/autoform.cxx:1019] -
[sc/source/core/tool/autoform.cxx:1029]: (performance) Variable 'bRet' is
reassigned a value before the old one has been used.
[sc/source/core/tool/autoform.cxx:1082] -
[sc/source/core/tool/autoform.cxx:1092]: (performance) Variable 'bRet' is
reassigned a value before the old one has been used.
[sc/source/core/tool/autoform.cxx:1107] -
[sc/source/core/tool/autoform.cxx:1110]: (performance) Variable 'bRet' is
reassigned a value before the old one has been used.
In some of these cases, the result of a test is retrieved in bRet but unused
even if there's a problem.
Any idea?
Julien
--
View this message in context:
http://nabble.documentfoundation.org/Wrong-indentation-which-leads-to-segfault-in-sc-source-ui-docshell-docfunc-cxx-tp4026726.html
Sent from the Dev mailing list archive at Nabble.com.
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: Wrong indentation which leads to segfault in sc/source/ui/docshell/docfunc.cxx
Julien,
Looks to me like a very nice catch ...
Here is the version as of 2011.06.15 showing braces on the for loops in both
then and else blocks...
from open grok --
http://opengrok.libreoffice.org/xref/core/sc/source/ui/docshell/docfunc.cxx?r=1b363f632110e80ead67ff376e92e4487556ca55
3735
3736 if (bSize)
3737 {
3738 SCCOLROW nCols[2] = { nStartCol, nEndCol };
3739 SCCOLROW nRows[2] = { nStartRow, nEndRow };
3740
3741 for (SCTAB nTab=0; nTabnTabCount; nTab++)
3742 if (aMark.GetTableSelect(nTab))
3743 {
3744 SetWidthOrHeight( sal_True, 1,nCols, nTab,
SC_SIZE_VISOPT, STD_EXTRA_WIDTH, false, sal_True);
3745 SetWidthOrHeight( false,1,nRows, nTab,
SC_SIZE_VISOPT, 0, false, false);
3746 rDocShell.PostPaint( 0,0,nTab,
MAXCOL,MAXROW,nTab,
3747 PAINT_GRID | PAINT_LEFT |
PAINT_TOP );
3748 }
3749 }
3750 else
3751 {
3752 for (SCTAB nTab=0; nTablt;nTabCount; nTab++)
3753 if (aMark.GetTableSelect(nTab))
3754 {
3755 sal_Bool bAdj = AdjustRowHeight(
ScRange(nStartCol, nStartRow, nTab,
3756 nEndCol,
nEndRow, nTab), false );
3757 if (bAdj)
3758 rDocShell.PostPaint( 0,nStartRow,nTab,
MAXCOL,MAXROW,nTab,
3759 PAINT_GRID | PAINT_LEFT
);
3760 else
3761 rDocShell.PostPaint( nStartCol, nStartRow,
nTab,
3762 nEndCol, nEndRow, nTab,
PAINT_GRID );
3763 }
3764 }
The next version (2011.07.05)
http://opengrok.libreoffice.org/xref/core/sc/source/ui/docshell/docfunc.cxx?r=2b88f6d32f572792597ccbb15276b9db52db7d10
is a commit labelled quot;change remaining manual loops to
ScMarkData::iterator quot; and the braces disappear on just one of the
loops. Because the braces were once there and, especially because the else
block has ~ same action within the braces, I say that they should definitely
come back. Even though, I am not at all clear what this code is doing
(whilst autoformatting selected cells in Calc)... this is an obvious oopsie
within a greater code cleanup that should be corrected.
This is not fdo# 47466 -- that crash gets close but doesn't quite get to
this code: that non-reproducible crash occurred inside the AutoFormat
function called just above the missing braces.
After some more looking at the code, it appears to be applying widths and
heights... Some testing shows that if one selects a tabular area and apply a
non-default width and height (Format-Rows/Columns-Height/Width) then and
uncheck AutoFormat Width and Height under AutoFormat... - More. Doing the
AutoFormat then forgets the width (doesn't loop) but not the height (still
loops).
The block of code in question starts to look like maybe it is fix code that
corrects an overactive AutoFormat function called just above.
This here bug -- https://bugs.freedesktop.org/show_bug.cgi?id=51524
Shows similar loss-of-width behavior in collapse/uncollapse (as opposed to
straight up AutoFormat)
There are also several reports of loss of height information (including
self-healing upon edit) - most of them are closed /or related to file
open/close. Maybe this block of W/H fix code is missing elsewhere, maybe it
is not needed here anymore, maybe the open save fix the values better than
this now, maybe this simply is the width/height portion of the AutoFormat
... h
I will check to see if inserting braces in the code fixes whatever
loss-of-width/height behavior that I can reproduce or find.
My 2cents... and now a couple more:
I note that, for all its type mushiness, BASIC will not allow this kind of
mistake. The then-clause must either be all on one line (can do IF test
THEN StmtA : StmtB : StmtC) and a multi-line then-clause must be terminated
with End If or Else.
Julien, I think you should get the patch credit - this is your find.
However, if it will help you continue to run cppchecks and find more of
these interesting puzzles, I would be happy to patch a pair of braces for
you and mark the appropriate bugs as resolved by you ;-D
Sweet catch,
LeMoyne
--
View this message in context:
http://nabble.documentfoundation.org/Wrong-indentation-which-leads-to-segfault-in-sc-source-ui-docshell-docfunc-cxx-tp4026726p4026751.html
Sent from the Dev mailing list archive at Nabble.com.
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: Wrong indentation which leads to segfault in sc/source/ui/docshell/docfunc.cxx
If only I could *always* read well ... and I didn't second guess myself so easily ... The stack trace from 47466 - Crash occurring in Windows 64bit versions... msvcr90!_invalid_parameter_noinfo+0xc sclo!std::_Treestd::_Tset_traitslt;short,std::lesslt;short,std::allocatorshort,0 ::const_iterator::operator*+0x35 [c:\program files\microsoft visual studio 9.0\vc\include\xtree @ 264] sclo!std::_Treestd::_Tset_traitslt;short,std::lesslt;short,std::allocatorshort,0 ::iterator::operator*+0xf [c:\program files\microsoft visual studio 9.0\vc\include\xtree @ 466] sclo!ScDocFunc::AutoFormat+0x520 [d:\losrc\3553\sc\source\ui\docshell\docfunc.cxx @ 3739] sclo!ScViewFunc::AutoFormat+0x70 [d:\losrc\3553\sc\source\ui\view\viewfun2.cxx @ 1575] Is almost identical to yours. You HAVE reproduced fdo#47466. The iterator functions appear to paper over the access at itr.end() everywhere except 1) Windows 64bit where it crashes and 2) your cppcheck which has replaced the standard iterator with a safe iterator, if I am not mistaken. But really the unasked for width adjustments people see could be the result of an uncaught exception shortcutting all the rest of the width/height fix, etc so format does not get restored to fixed width and height during the AutoFormat. Bubbling exception from this could also help explain the report of related undo failures after the errant width/height change as in fdo# 34552 EDITING: Calc loses row height value when modifying a cell https://bugs.freedesktop.org/show_bug.cgi?id=34552 -- if it skips out from the point of the missing braces then it will fail to record the undo info for the width height change and that could cause the behavior there And maybe even other bugs if calls to this AutoFormat function are involved there ... Like https://bugs.freedesktop.org/show_bug.cgi?id=57176 Undo of delete of Conditional Formatting works for one cell but fails for multi-cell (cf. if (bSize) ...) On final note, I don't think that bool ScDocFunc::AutoFormat( const ScRange rRange, ... will return bSuccess = true under any circumstances. Just sayin' ;-) I will seek more bugs and test them before and after the brace insert in the next few days ... Again... looks like a real good catch here. Gratz and thank you for the puzzle. Happy New Year !!! LeMoyne -- View this message in context: http://nabble.documentfoundation.org/Wrong-indentation-which-leads-to-segfault-in-sc-source-ui-docshell-docfunc-cxx-tp4026726p4026757.html Sent from the Dev mailing list archive at Nabble.com. ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
