Bug 433437 - FreeBSD support, part 1
Summary: FreeBSD support, part 1
Status: RESOLVED FIXED
Alias: None
Product: valgrind
Classification: Developer tools
Component: vex (show other bugs)
Version: unspecified
Platform: Other FreeBSD
: NOR normal
Target Milestone: ---
Assignee: Julian Seward
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-02-22 18:05 UTC by Paul Floyd
Modified: 2021-10-06 07:33 UTC (History)
4 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
diff in vex directory (29.07 KB, patch)
2021-02-22 18:05 UTC, Paul Floyd
Details
diff in vex directory (27.29 KB, patch)
2021-03-23 13:19 UTC, Paul Floyd
Details
diff in vex directory (27.29 KB, patch)
2021-09-23 19:12 UTC, Paul Floyd
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Paul Floyd 2021-02-22 18:05:43 UTC
Created attachment 136050 [details]
diff in vex directory

Primarily these changes concern the nature of alignment generated signals. On Linux, these produce SIGSEGV and n FreeBSD they produce SIGBUS.

It would be cleaner in my opinion to rename the functions to be

gen_SIGNAL_if_not_XX_aligned
gen_SIGNAL_if_not_16_aligned
etc.

and just have the different code as follows

static
void gen_SIGNAL_if_not_XX_aligned ( IRTemp effective_addr, ULong mask )
{
   stmt(
      IRStmt_Exit(
         binop(Iop_CmpNE64,
               binop(Iop_And64,mkexpr(effective_addr),mkU64(mask)),
               mkU64(0)),
#if !defined(VGO_FREEBSD)
         Ijk_SigSEGV,
#else
         Ijk_SigBUS,
#endif
         IRConst_U64(guest_RIP_curr_instr),
         OFFB_RIP
      )
   );
}
Comment 1 Paul Floyd 2021-02-22 18:08:10 UTC
Forgot to add, there are also a few minor nits that clang was moaning about.
Comment 2 Guy Harris 2021-03-23 01:57:11 UTC
(In reply to Paul Floyd from comment #0)
> Created attachment 136050 [details]
> diff in vex directory
> 
> Primarily these changes concern the nature of alignment generated signals.
> On Linux, these produce SIGSEGV and n FreeBSD they produce SIGBUS.

On most if not all *BSDs they produce SIGBUS.  That *might* be the case on Solaris as well.  (I remember it being the case on SunOS on Sun-2, where 16-bit and wider quantities had to be aligned on 16-bit boundaries, and on SPARC; that probably carried over to SunOS 5.)  Personally, I think that's how it *should* be, but....
Comment 3 Paul Floyd 2021-03-23 09:36:57 UTC
I'm fairly sure that this behaviour is not in SunOS 5 amd64. Work was done on a SPARC port but it never made it to the official repo (this was around the time that Orcale dropped the ball). Valgrind does run on Solaris 11.3, 11.4 and Illumos amd64. I'll double check next time I boot my trusty old Solaris PC.
Comment 4 Tom Hughes 2021-03-23 09:52:29 UTC
If you think it would be better to rename the function and have a single ifdef there (and I definitely agree that it would be) then why not post that patch rather than one that you think is suboptimal?

Changing the function name would reduce code duplication (virtually all of the SEGV vs BUS functions are the same) and also reduce the amount of ifdefery dramatically.
Comment 5 Paul Floyd 2021-03-23 10:05:08 UTC
In general I've tried to tread very carefully and make as few changes as possible that would affect Linux. This probably took that too far. I'll update the code and the patch.
Comment 6 Paul Floyd 2021-03-23 13:19:26 UTC
Created attachment 136983 [details]
diff in vex directory

Change to using generic SIGNAL functions for all platforms, and put the #ifdefed code inside gen_SIGNAL_if_not_XX_align
Comment 7 Paul Floyd 2021-09-23 19:12:39 UTC
Created attachment 141832 [details]
diff in vex directory
Comment 8 Mark Wielaard 2021-09-26 14:50:04 UTC
I cannot comment on the freebsd specific changes, I assume they are correct.

The changes in VEX/priv/guest_amd64_toIR.c seem fine.
As do the changes in VEX/priv/host_amd64_defs.c and VEX/priv/host_amd64_isel.c.
MIPS and PPC already use synth_sigbus, so this should work.
The change to the initialization of vex_control is correct.
The change in VEX/pub/libvex.h looks OK too to help clang.

The patch compiles fine on Fedora x86_64, please apply.
Comment 9 Paul Floyd 2021-10-06 07:33:45 UTC
These changes pushed with commit 33012dd82ba5f3502d3c6c5d95cf22b7af48f6b3