flintlib / flint

FLINT (Fast Library for Number Theory)
http://www.flintlib.org
GNU Lesser General Public License v3.0
410 stars 238 forks source link

Valgrind reports unhandled instruction #1677

Open langefa opened 7 months ago

langefa commented 7 months ago

When running Valgrind on an example for the FUEL library (https://gitlab.com/feynmanintegrals/fuel), it encounters an unhandled instruction:

==903142== Memcheck, a memory error detector
==903142== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
==903142== Using Valgrind-3.21.0 and LibVEX; rerun with -h for copyright info
==903142== Command: ./test --calc flint --file example_coeffs/huge_but_short.data
==903142== 
vex amd64->IR: unhandled instruction bytes: 0x62 0x73 0xB5 0x48 0x3A 0xD4 0x1 0xC4 0x43 0xD
vex amd64->IR:   REX=0 REX.W=0 REX.R=0 REX.X=0 REX.B=0
vex amd64->IR:   VEX=0 VEX.L=0 VEX.nVVVV=0x0 ESC=NONE
vex amd64->IR:   PFX.66=0 PFX.F2=0 PFX.F3=0
==903142== valgrind: Unrecognised instruction at address 0x513fd46.
==903142==    at 0x513FD46: mpoly_void_ring_init_fmpz_mpoly_ctx (void_ring.c:113)
==903142==    by 0x5123EEA: fmpz_mpoly_set_str_pretty (set_str_pretty.c:24)
==903142==    by 0x14B359: rational_function (ratfunc_flint.cpp:1212)
...

This did not happen with v3.0.1. However, to fix #1652, FUEL was updated to commit 3ef8253157dd22b1480f7669dbb3684008637ded. I checked that the problem persists with the most recent commit 2318e1bebd5df5723db7e048494ce31bc4baff61 in trunk.

I noticed this in the context of looking into a memory leak in FUEL, see https://gitlab.com/feynmanintegrals/fuel/-/issues/1 and #1676.

albinahlback commented 7 months ago

I'm bad at valgrind -- do I understand it correctly that the unrecognized instruction is in fmpz_mpoly/void_ring.c at line 113 in FLINT?

albinahlback commented 7 months ago

What CFLAGS were used when you compiled it with 3.0.1, and what CFLAGS are being used now? Are they the same or not?

Sander80 commented 7 months ago

I'm bad at valgrind -- do I understand it correctly that the unrecognized instruction is in fmpz_mpoly/void_ring.c at line 113 in FLINT?

Yes, it pritnt a backtrace of calls, the top line is the inner call in the stack

Sander80 commented 7 months ago

Unless @langefa changed something, it is should be by default, and the flags should be the same. We only pass paths to gmp and mpfr

albinahlback commented 7 months ago

Okay, so the default CFLAGS where changes some time after that. We increased the optimization level from -O2 to -O3 and added -march=native (unless cross-compiling). It may be the case that Valgrind simply does not recognize the instruction that were generated (see https://stackoverflow.com/questions/37032339/valgrind-unrecognised-instruction for example).

Sander80 commented 7 months ago

Is there any recomended way to force specific CFLAGS? I will force their usage in FUEL, so that @langefa can retry (I have not seen the error message)

albinahlback commented 7 months ago

Assuming you are using Make to build FLINT, yes. For instance,

$ ./configure CFLAGS="-O2"

will set CFLAGS to -O2, and there may be some other default options that may be put in there too (for instance, --enable-debug is the default, so -g will also be added to the CFLAGS unless you specify --disable-debug).

Sander80 commented 7 months ago

@albinahlback , thank ypu @langefa , I pushed a commit so that fuel will configure flint with -O2, please give it a try

langefa commented 7 months ago

The CFLAGS are indeed the culprit. -O3 has no impact though, -march=native is solely responsible. It looks like there is some architecture dependent optimization on my machine that is not compatible with Valgrind.

Sander80 commented 7 months ago

What is the resolution then? Nothing wrong here in flint, the issue can be closed? I am probably then returning default opitons and you can change them if needed

fredrik-johansson commented 7 months ago

This is a common problem with valgrind. Before closing the issue, we should add a section to the FLINT docs on using valgrind including a mention of this problem.

Sander80 commented 7 months ago

I see, then it's good we reported it.