[Bug c++/104243] New: Optimization requires __sync_synchronize

[Bug c++/104243] New: Optimization requires __sync_synchronize

[aklitzing at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104243

            Bug ID: 104243
           Summary: Optimization requires __sync_synchronize
           Product: gcc
           Version: 6.4.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: aklitzing at gmail dot com
  Target Milestone: ---

Hi,

long time ago I found a strange behaviour with Qt and GCC 6.3. I tried it again
today with GCC 11.1.0.

If I compile this snippet with an optimization level it will output "broken".
If I use -O0 or use clang it will output "ok".
If I add "__sync_synchronize" it will work with any optimization, too. The
variable "idx" isn't updated in if-clause but it seems updated if qDebug is
called.


#include <QVersionNumber>
#include <QDebug>

int main(int argc, char** argv)
{
        const QString pVersion = QStringLiteral("1.12.2");
        int idx = 0;
        const auto& version = QVersionNumber::fromString(pVersion, &idx);

        // #ifdef Q_CC_GNU
        // __sync_synchronize();
        // #endif

        if(idx == 0)
                qDebug() << "broken";
        else
                qDebug() << "ok";

        qDebug() << "idx:" << idx;
        return 0;
}

$ g++ -O2 example.cpp `pkg-config --libs --cflags Qt5Core`
$ ./a.out
broken
idx: 6

$ g++ -O0 bla.cpp `pkg-config --libs --cflags Qt5Core`
$ ./a.out
ok
idx: 6


Source of QVersionNumber:
https://code.qt.io/cgit/qt/qtbase.git/tree/src/corelib/tools/qversionnumber.cpp?h=5.15.2#n463

Qt-Report:
https://bugreports.qt.io/browse/QTBUG-62185

[Bug c++/104243] Optimization requires __sync_synchronize

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104243

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |WAITING
   Last reconfirmed|                            |2022-01-26
     Ever confirmed|0                           |1

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
Please provide preprocessed source, QVersionNumber doesn't seem to be available
for me.

[Bug c++/104243] Optimization requires __sync_synchronize

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104243

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |marxin at gcc dot gnu.org

--- Comment #2 from Martin Liška <marxin at gcc dot gnu.org> ---
It's related to fact that the following function is pure:

    [[nodiscard]] __attribute__((visibility("default"))) static
__attribute__((pure)) QVersionNumber fromString(const QString &string, int
*suffixIndex = nullptr);

    [[nodiscard]] __attribute__((visibility("default"))) static
__attribute__((pure)) QVersionNumber fromString(QLatin1String string, int
*suffixIndex = nullptr);
    [[nodiscard]] __attribute__((visibility("default"))) static
__attribute__((pure)) QVersionNumber fromString(QStringView string, int
*suffixIndex = nullptr);

and thus we optimize out: qt.ii.037t.fre1

Replaced idx with 0 in all uses of idx.0_1 = idx;
Removing unexecutable edge from if (idx.0_1 == 0)
Removing dead stmt idx.0_1 = idx;

[Bug c++/104243] Optimization requires __sync_synchronize

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104243

--- Comment #3 from Martin Liška <marxin at gcc dot gnu.org> ---
Thus I think the bug tens to be invalid..

[Bug c++/104243] Optimization requires __sync_synchronize

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104243

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |INVALID
             Status|WAITING                     |RESOLVED

--- Comment #4 from Martin Liška <marxin at gcc dot gnu.org> ---
Simpler reproducer:

$ cat repro.cc
int
__attribute__((pure))
foo(int *x);

int main()
{
  int a;
  int r = foo (&a);

  if (a != 123)
    __builtin_abort ();
}
$ cat repro2.cc
int
__attribute__((pure))
foo(int *x)
{
  *x = 123;
  return 0;
}

$ gcc repro*.cc -O2 && ./a.out
Aborted (core dumped)

[Bug c++/104243] Optimization requires __sync_synchronize

[aklitzing at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104243

--- Comment #5 from A. Klitzing <aklitzing at gmail dot com> ---
Created attachment 52294
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=52294&action=edit
assembler output

By the way... If idx is not initialized it will work, too.

@@ -4,7 +4,7 @@
 int main(int argc, char** argv)
 {
         const QString pVersion = QStringLiteral("1.12.2");
-        int idx = 0;
+        int idx;
         const auto& version = QVersionNumber::fromString(pVersion, &idx);

         // #ifdef Q_CC_GNU

[Bug c++/104243] Optimization requires __sync_synchronize

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104243

--- Comment #6 from Martin Liška <marxin at gcc dot gnu.org> ---
(In reply to A. Klitzing from comment #5)
> Created attachment 52294 [details]
> assembler output
> 
> By the way... If idx is not initialized it will work, too.
> 
> @@ -4,7 +4,7 @@
>  int main(int argc, char** argv)
>  {
>          const QString pVersion = QStringLiteral("1.12.2");
> -        int idx = 0;
> +        int idx;
>          const auto& version = QVersionNumber::fromString(pVersion, &idx);
>  
>          // #ifdef Q_CC_GNU

You were lucky in this case ;)

Anyway, upstream removed the pure attribute as we suggested:
https://codereview.qt-project.org/c/qt/qtbase/+/392357

[Bug c++/104243] Optimization requires __sync_synchronize

[thiago at kde dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104243

--- Comment #7 from Thiago Macieira <thiago at kde dot org> ---
(In reply to Martin Liška from comment #6)
> Anyway, upstream removed the pure attribute as we suggested:
> https://codereview.qt-project.org/c/qt/qtbase/+/392357

Can we be assured the pure attribute will work for complex return types?

https://gcc.godbolt.org/z/KE4s74od3
struct S
{
    bool *ptr;
    S();
    S(const S &);
};

#ifdef __GNUC__
__attribute__((pure))
#endif
S f1();
bool f2()
{
    return *f1().ptr;
}

[Bug c++/104243] Optimization requires __sync_synchronize

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104243

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rguenth at gcc dot gnu.org

--- Comment #8 from Martin Liška <marxin at gcc dot gnu.org> ---
@Richi: Can you chime in, please?

[Bug c++/104243] Optimization requires __sync_synchronize

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104243

--- Comment #9 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Thiago Macieira from comment #7)
> (In reply to Martin Liška from comment #6)
> > Anyway, upstream removed the pure attribute as we suggested:
> > https://codereview.qt-project.org/c/qt/qtbase/+/392357
> 
> Can we be assured the pure attribute will work for complex return types?
> 
> https://gcc.godbolt.org/z/KE4s74od3
> struct S
> {
>     bool *ptr;
>     S();
>     S(const S &);
> };
> 
> #ifdef __GNUC__
> __attribute__((pure))
> #endif
> S f1();
> bool f2()
> {
>     return *f1().ptr;
> }

I think this came up before.  Yes, the intent is that this is well-defined
if 'f1' has no observable effects on the state of the program other than
to return a value.  There might have been bugs in old versions of GCC
of course - I do remember some points-to analysis oddities around this,
checking GCC 7 it works OK there at least.  A testcase to check would be
for example

struct S
{
  bool *ptr;
  S();
  S(const S &);
};

__attribute__((pure))
S f1(bool *);
void f2()
{
  bool c = false;
  *f1(&c).ptr = true;
  if (!c)
    __builtin_abort ();
}

but all versions godbolt has do not miscompile this.