[PATCH] c++ bugfix: inconsistent runtime beahavior when have -O2

[PATCH] c++ bugfix: inconsistent runtime beahavior when have -O2

[Hongbo Liu]

[-- Attachment #1: Type: text/plain, Size: 1152 bytes --]

Hi,

This is my first time using email patch, please correct me if there is any error. Thanks!


I found a c++ optimization bug first introduced via commit 520d5ad337eaa15860a5a964daf7ca46cf31c029, which will make program compiled with -O2 have unexpected behavior.


How to produce


1. Checkout to latest master commit: c4d702fb3c1e2f6e1bc8711da81bff59543b1b19
2. Build and install the gcc;
3. Compile the attached file `test.cc` with command: `latest-g++ -std=c++20 -g0 -O1 test.cc -o test_o1`;
4. The output of `./test_o1` will be `offset delta: 5`;
5.  Compile the attached file `test.cc` with command: `latest-g++ -std=c++20 -g0 -O2 test.cc -o test_o2`;
6. The output of `./test_o2` will be `offset delta: 0`;


The program behavior with `-O2` is inconsistent with `-O1` and unexpected.


How to fix


The bug was introduced via commit 520d5ad337eaa15860a5a964daf7ca46cf31c029. The attached patch file could fix the problem and make `-O2` and `-O1` have same behavior.


Request for suggestion


I could not find and document about how to add this kind of test, could anyone give me some suggestions?

[-- Attachment #2: test.cc --]
[-- Type: application/octet-stream, Size: 806 bytes --]

#include <cstdint>
#include <iostream>
#include <algorithm>
#include <vector>

using namespace std;

void parse_varint_u64(void **buf, uint64_t *val) {
	size_t of = 0;
	uint64_t num = 0;

	do {
		num |= (uint64_t)(((char *)(*buf))[(int)of] & 0x7f);
	} while (((char *)(*buf))[(int)of++] & 0x80);

	*val = num;
}

void parse_varint_i64(void **buf, int64_t *val) {
	uint64_t n = 123;
	parse_varint_u64(buf, &n);

  // printf("n: %d\n", n);

  *val = (int64_t)(n >> 1) ^ -(int64_t)(n & 1);
}

int main(int argc, char *argv[]) {
  size_t size = 16;
  void* data = malloc(size);
  for (int i = 0; i < size; ++i) {
    ((char*)data)[i] = i + 10;
  }

	long long offset_delta = 0;

  parse_varint_i64(&data, (int64_t *)&offset_delta);

  std::cout << "offset delta: " << offset_delta << std::endl;

  return 0;
}

[-- Attachment #3: 0001-PATCH-c-bugfix-inconsistent-logic-when-have-O2.patch --]
[-- Type: application/octet-stream, Size: 698 bytes --]

From 2cfe523be931f26aa9988fa4d0b8ca57be548517 Mon Sep 17 00:00:00 2001
From: HongboLiu <lhbf@qq.com>
Date: Mon, 6 Jun 2022 16:49:32 +0800
Subject: [PATCH] [PATCH] c++ bugfix: inconsistent logic when have -O2

---
 gcc/gimple.cc | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/gcc/gimple.cc b/gcc/gimple.cc
index b70ab4d..d9593aa 100644
--- a/gcc/gimple.cc
+++ b/gcc/gimple.cc
@@ -1566,6 +1566,9 @@ gimple_call_arg_flags (const gcall *stmt, unsigned arg)
   attr_fnspec fnspec = gimple_call_fnspec (stmt);
   int flags = 0;
 
+  if (!fnspec.known_p ())
+    return 0;
+
   if (fnspec.known_p ())
     flags = fnspec.arg_eaf_flags (arg);
   tree callee = gimple_call_fndecl (stmt);
-- 
2.7.4

Re: [PATCH] c++ bugfix: inconsistent runtime beahavior when have -O2

[Andrew Pinski]

On Mon, Jun 6, 2022 at 2:12 AM Hongbo Liu via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Hi,
>
> This is my first time using email patch, please correct me if there is any error. Thanks!
>
>
> I found a c++ optimization bug first introduced via commit&nbsp;520d5ad337eaa15860a5a964daf7ca46cf31c029, which will make program compiled with -O2 have unexpected behavior.

Except your testcase violates C/C++ aliasing rules.
You store to offset_delta via int64_t (which is long on x86_64) but
then do a load via long long.
If you fix the testcase, it works correctly at -O2 and above. If you
don't want strict aliasing to be turned on you can use
-fno-strict-aliasing.

Your patch is wrong as now none of the modref data will be used which
is exposing the bug in the testcase and it is a bug in the source code
you are compiling.

As for how to add the testcase,
https://gcc.gnu.org/wiki/HowToPrepareATestcase does have some
information.
So does https://gcc.gnu.org/onlinedocs/gccint/Testsuites.html#Testsuites .


Thanks,
Andrew Pinski

>
>
> How to produce
>
>
> 1. Checkout to latest master commit:&nbsp;c4d702fb3c1e2f6e1bc8711da81bff59543b1b19
> 2. Build and install the gcc;
> 3. Compile the attached file `test.cc` with command: `latest-g++ -std=c++20 -g0 -O1 test.cc -o test_o1`;
> 4. The output of `./test_o1` will be `offset delta: 5`;
> 5.&nbsp; Compile the attached file `test.cc` with command: `latest-g++ -std=c++20 -g0 -O2 test.cc -o test_o2`;
> 6. The output of `./test_o2` will be `offset delta: 0`;
>
>
> The program behavior with `-O2` is inconsistent with `-O1` and unexpected.
>
>
> How to fix
>
>
> The bug was introduced via commit 520d5ad337eaa15860a5a964daf7ca46cf31c029. The attached patch file could fix the problem and make `-O2` and `-O1` have same behavior.
>
>
> Request for suggestion
>
>
> I could not find and document about how to add this kind of test, could anyone give me some suggestions?