Re: A coverity bug for Tommaso

[Richard Heck]

On 12/16/2016 04:51 PM, Jean-Marc Lasgouttes wrote:
> Le 16/12/16 à 20:34, Richard Heck a écrit :
>> Doesn't this just mean that the loop logic can be removed? I.e., just
>> delete the for statement? It has no effect, since there is no
>> initialization clause.
>
> There is some code before the for() loop:
>
> // Try all possible regexp matches,
> //until one that verifies the braces match test is found
> regex const *p_regexp = at_begin ?  : 
> sregex_iterator re_it(str.begin(), str.end(), *p_regexp);
> sregex_iterator re_it_end;
> for (; re_it != re_it_end; ++re_it) {

Right. And re_it_end is never initialized, here or later.

rh

Re: #10481: Hardening LyX against potential misuse

[Pavel Sanda]

Helge Hafting wrote:
> Protection will not be achieved in most cases, because users are used to 

While I agree with what you write in general about security, I do not think
this is how things were implemented, so in 'most cases' you are wrong.

1. Unless you do informed decision and go to the prefs and allow dangerous
   mode you will never be asked and nothing will ever run.
   This covers 99% of lyx users and usecases.

2. If you are in special need for knitr/gnuplot you can allow it personally
   for yourself or ask your colleagues to do that as well for shared document.
   
   Here your concern applies and here we might differ - I think that from this
   point it's basically your responsibility to check what you are opening and
   do not suggest colleagues who don't understand what they are doing to use
   this feature.
   If people think that (2.) is still dangerous we might hide it even
   more so only hackers can use it, but personally I do not see need for it.

3. Chrooting is nice idea and practically hard to achieve across platforms.
   Many years back when I checked gnuplot devs were against including 'safe
   mode' so the disable-write18-in-LaTeX alternative for gnuplot is not
   in our reach either.

Pavel

Re: A coverity bug for Tommaso

[Jean-Marc Lasgouttes]

Le 16/12/16 à 20:34, Richard Heck a écrit :

Doesn't this just mean that the loop logic can be removed? I.e., just
delete the for statement? It has no effect, since there is no
initialization clause.


There is some code before the for() loop:

// Try all possible regexp matches,
//until one that verifies the braces match test is found
regex const *p_regexp = at_begin ?  : 
sregex_iterator re_it(str.begin(), str.end(), *p_regexp);
sregex_iterator re_it_end;
for (; re_it != re_it_end; ++re_it) {

JMarc

Re: A coverity bug for Tommaso

[Richard Heck]

On 12/16/2016 11:02 AM, Jean-Marc Lasgouttes wrote:
> Dear Tommaso,
>
> Since you are around again, could you have a look at
> MatchStringAdv::findAux()? The big else branch says:
>
> for (; re_it != re_it_end; ++re_it) {
>  913match_results
> const & m = *re_it;
>  914// Check braces on the segment that
> matched the entire regexp expression,
>  915// plus the last subexpression, if a (.*?)
> was inserted in the constructor.
>  916if (!braces_match(m[0].first, m[0].second,
> open_braces))
>  917return 0;
>  918// Check braces on segments that matched
> all (.*?) subexpressions,
>  919// except the last "padding" one inserted
> by lyx.
>  920for (size_t i = 1; i < m.size() - 1; ++i)
>  921if (!braces_match(m[i].first,
> m[i].second))
>  922return false;
>  923// Exclude from the returned match length
> any length
>  924// due to close wildcards added at end of
> regexp
>  925if (close_wildcards == 0)
>  926return m[0].second - m[0].first;
>  927else
>  928return m[m.size() -
> close_wildcards].first - m[0].first;
>  929}
>
> As you can see, all the cases lead to early return in the first loop
> run. I have no idea of how to work around this.

Doesn't this just mean that the loop logic can be removed? I.e., just
delete the for statement? It has no effect, since there is no
initialization clause.

Richard

A coverity bug for Tommaso

[Jean-Marc Lasgouttes]

Dear Tommaso,

Since you are around again, could you have a look at 
MatchStringAdv::findAux()? The big else branch says:


for (; re_it != re_it_end; ++re_it) {
 913match_results const 
& m = *re_it;
 914// Check braces on the segment that matched 
the entire regexp expression,
 915// plus the last subexpression, if a (.*?) 
was inserted in the constructor.
 916if (!braces_match(m[0].first, m[0].second, 
open_braces))

 917return 0;
 918// Check braces on segments that matched 
all (.*?) subexpressions,
 919// except the last "padding" one inserted 
by lyx.

 920for (size_t i = 1; i < m.size() - 1; ++i)
 921if (!braces_match(m[i].first, 
m[i].second))

 922return false;
 923// Exclude from the returned match length 
any length
 924// due to close wildcards added at end of 
regexp

 925if (close_wildcards == 0)
 926return m[0].second - m[0].first;
 927else
 928return m[m.size() - 
close_wildcards].first - m[0].first;

 929}

As you can see, all the cases lead to early return in the first loop 
run. I have no idea of how to work around this.


JMarc

Re: LyX 2.2 slowness

[Jean-Marc Lasgouttes]

Le 09/12/2016 à 15:58, Jean-Marc Lasgouttes a écrit :

Le 07/12/2016 à 16:10, Jean-Marc Lasgouttes a écrit :

Le 07/12/2016 à 12:15, Jean-Marc Lasgouttes a écrit :

I'll post a new version to try soon.


Here is a patch to play with. It is not perfect, but I would be
interested to know whether it improves performance with Qt4.


I have improved a bit the patch (fixed a couple bugs)  and now it also
comes with built-in profiling.
Just comment-out the
#define DISABLE_PMPROF
at the beginning to enable profiling of the 3 main caches.

Here is what I get on User Guide by using PageDown over the whole file
(not very good for the cache)



Here is the same test with Qt5:

#pmprof# getTextLayout: 19.07usec, count=19115, total=364.61msec
   hit: 53%, 4.17usec, count=10135, total=42.25msec
  miss: 46%, 35.90usec, count=8980, total=322.35msec
#pmprof# breakAt: 42.10usec, count=3981, total=167.60msec
   hit: 69%, 3.44usec, count=2762, total=9.51msec
  miss: 30%, 129.69usec, count=1219, total=158.09msec
#pmprof# width: 6.09usec, count=153658, total=936.00msec
   hit: 89%, 0.47usec, count=137232, total=64.39msec
  miss: 10%, 53.06usec, count=16426, total=871.61msec

Note that these numbers are misleading. Indeed, since Qt5 does its own 
caching, the times in case of "miss" take in account the fact that there 
is a miss also in the lower level Qt caching. Therefore the miss times 
are artificially large.


To have a better view of the effect of the cache, one can run the same 
test with cache disabled (comment out #define CACHE_FONT_METRICS in 
updated patch in attachment).


#pmprof# getTextLayout: 17.09usec, count=19224, total=328.52msec
#pmprof# breakAt: 76.84usec, count=4074, total=313.06msec
#pmprof# width: 17.18usec, count=152237, total=2614.93msec

These numbers show that the effect is actually slightly negative for 
getTextLayout. We might want though to skip the caching of QTextLayout 
on Qt5 (moreover they can use a lot of memory). The two other ones seem 
good to keep.


I'd be interested to see other tests, especially on MacOS and Windows.

JMarc
>From bd638c0c3ced186b81d7161042b62c5439b5382b Mon Sep 17 00:00:00 2001
From: Jean-Marc Lasgouttes 
Date: Tue, 5 Jul 2016 14:06:22 +0200
Subject: [PATCH] Add caching for the QTextLayout objects we use

The QTextLayout handling is terribly slow on Qt 4.8.7, but some
caching has been added in Qt5 that makes it much faster. For some
reason, it is not that slow with Qt 4.8.1.

This commit introduces some caching, which should probably only be
active for Qt 4. This caching only happens when CACHE_FONT_METRICS is
defined.

[NOTE: this version has profiling hooks, enabled by commenting out the line
  #define DISABLE_PMPROF
that should eventually be removed.]

Improve the caching of QTextLayout objects used for pos2x and x2pos
and use them for drawing too. We originally used a trivial caching
of the last QTextLayout, but now they are kept in a QCache.
Moreover, caching is enabled for these QTextLayout object (not sure
what this does).

This patch also adds some caching in the breakAt method, the only user of
QTextLayout which did not have some kind of caching already.

Finally the cached QTextLayout objects are also used by the painter
for drawing text.

For some weird reasons related to Argument-dependent look-up, the
qHash(docstring) function has to be defined in std namespace, since
lyx::docstring is actually std::basic_string.
---
 src/frontends/qt4/GuiFontMetrics.cpp | 156 +--
 src/frontends/qt4/GuiFontMetrics.h   |  33 +---
 src/frontends/qt4/GuiPainter.cpp |  13 ++-
 src/support/convert.cpp  |   7 ++
 4 files changed, 151 insertions(+), 58 deletions(-)

diff --git a/src/frontends/qt4/GuiFontMetrics.cpp b/src/frontends/qt4/GuiFontMetrics.cpp
index d3c89f1..ff61481 100644
--- a/src/frontends/qt4/GuiFontMetrics.cpp
+++ b/src/frontends/qt4/GuiFontMetrics.cpp
@@ -21,11 +21,36 @@
 
 #include "insets/Inset.h"
 
+#include "support/convert.h"
 #include "support/lassert.h"
 
+//#define DISABLE_PMPROF
+#include "support/pmprof.h"
+
+#ifdef CACHE_FONT_METRICS
+#include 
+#endif
+
 using namespace std;
 using namespace lyx::support;
 
+#ifdef CACHE_FONT_METRICS
+namespace std {
+
+/*
+ * Argument-dependent lookup implies that this function shall be
+ * declared in the namespace of its argument. But this is std
+ * namespace, since lyx::docstring is just std::basic_string.
+ */
+uint qHash(lyx::docstring const & s)
+{
+	return qHash(QByteArray(reinterpret_cast(s.data()),
+	s.size() * sizeof(lyx::docstring::value_type)));
+}
+
+}
+#endif
+
 namespace lyx {
 namespace frontend {
 
@@ -51,11 +76,17 @@ inline QChar const ucs4_to_qchar(char_type const ucs4)
 } // anon namespace
 
 
-// Limit strwidth_cache_ size to 512kB of string data
+/*
+ * Limit (strwidth|breakat)_cache_ size to 512kB of string data.
+ * Limit qtextlayout_cache_ size to 500 elements (we do not know the
+ * size of the 

Re: [LyX/master] Extend quote-insert

[Scott Kostyshak]

On Fri, Dec 16, 2016 at 01:04:13PM +0100, Jürgen Spitzmüller wrote:
> 2016-12-16 12:39 GMT+01:00 Scott Kostyshak :

> I'd like to list the choices explicitly.

OK.

Scott


signature.asc
Description: PGP signature

Re: [LyX/master] Extend quote-insert

[Jürgen Spitzmüller]

2016-12-16 12:39 GMT+01:00 Scott Kostyshak :

> On Fri, Dec 16, 2016 at 11:24:18AM +0100, Juergen Spitzmueller wrote:
>
> > + QuoteLanguage ql = EnglishQuotes;
> > +
> > + if (s == "english")
> > + ql = EnglishQuotes;
> > + else if (s == "swedish")
> > + ql = SwedishQuotes;
>
> Why not simplify to the following?
>
> QuoteLanguage ql = EnglishQuotes;
>
> if (s == "swedish")
> ql = SwedishQuotes;
>

I'd like to list the choices explicitly.

Jürgen


>
> Scott
>

Re: [LyX/master] Extend quote-insert

[Scott Kostyshak]

On Fri, Dec 16, 2016 at 11:24:18AM +0100, Juergen Spitzmueller wrote:

> + QuoteLanguage ql = EnglishQuotes;
> + 
> + if (s == "english")
> + ql = EnglishQuotes;
> + else if (s == "swedish")
> + ql = SwedishQuotes;

Why not simplify to the following?

QuoteLanguage ql = EnglishQuotes;

if (s == "swedish")
ql = SwedishQuotes;

Scott


signature.asc
Description: PGP signature