Bug 433439 - FreeBSD support, part 3
Summary: FreeBSD support, part 3
Status: RESOLVED FIXED
Alias: None
Product: valgrind
Classification: Developer tools
Component: general (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:21 UTC by Paul Floyd
Modified: 2021-10-06 21:07 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
diff in include directory (163.07 KB, patch)
2021-02-22 18:21 UTC, Paul Floyd
Details
diff in include directory (162.98 KB, patch)
2021-09-23 19:20 UTC, Paul Floyd
Details
freebsd3 patch (161.96 KB, text/plain)
2021-09-26 18:42 UTC, Mark Wielaard
Details
diff in include directory (162.13 KB, patch)
2021-10-05 20:48 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:21:19 UTC
Created attachment 136052 [details]
diff in include directory

Several new headers.
Add FreeBSD where there are typedefs for VG types
Add SONAMES for FreeBSD shared libs
Comment 1 Paul Floyd 2021-09-23 19:20:24 UTC
Created attachment 141834 [details]
diff in include directory
Comment 2 Mark Wielaard 2021-09-26 18:32:26 UTC
I think the new NSegment field in include/pub_tool_aspacemgr.h should be guarded by:

#if defined(VGO_freebsd)
      Bool    isFF;     // True --> is a fixed file mapping
#endif

As it is only used by freebsd.

The types in include/pub_tool_basics.h seem properly guarded.

The &qq_option[2] in include/pub_tool_options.h looks correct.

Adding const to the VG_(sysctl) void *newp would break some code and seems unnecessary.

The comment and inclusion of vki/vki-scnums-freebsd.h in include/pub_tool_vkiscnums.h seems unneeded.

The new vki files are only used by freebsd and I assume they describe things correctly.
Comment 3 Mark Wielaard 2021-09-26 18:42:26 UTC
Created attachment 141932 [details]
freebsd3 patch

This variant of the patch, with the changes suggested in my previous comment, works for me on fedora x86_64.
Comment 4 Paul Floyd 2021-09-26 20:13:11 UTC
Why do you think making sysctl newp const might be unsafe?

The signature on FreeBSD is

     int
     sysctl(const int *name, u_int namelen, void *oldp, size_t *oldlenp,
         const void *newp, size_t newlen);

On Darwin it is non--const, but there are only two calls to it both with NULL arguments for newp.

Either way newp gets cast to (UWord).

I prefer to keep it const as this makes the interface clearer (an optional input-only argument).


I've removed the include of vki/vki-scnums-freebsd.h in include/pub_tool_vkiscnums.h. It does no longer seem necessary.
Comment 5 Mark Wielaard 2021-09-26 21:29:15 UTC
(In reply to Paul Floyd from comment #4)
> Why do you think making sysctl newp const might be unsafe?
> 
> The signature on FreeBSD is
> 
>      int
>      sysctl(const int *name, u_int namelen, void *oldp, size_t *oldlenp,
>          const void *newp, size_t newlen);
> 
> On Darwin it is non--const, but there are only two calls to it both with
> NULL arguments for newp.
> 
> Either way newp gets cast to (UWord).
> 
> I prefer to keep it const as this makes the interface clearer (an optional
> input-only argument).

But it doesn't match the actual VG_(sysctl) function in coregrind/m_libcproc.c we actually use, so it causes compile errors.
Comment 6 Paul Floyd 2021-09-27 06:45:59 UTC
For sysctl the problem is that I created separate patches for include and coregrind (patch 12)

The definition is

Int VG_(sysctl)(Int *name, UInt namelen, void *oldp, SizeT *oldlenp, const void *newp, SizeT newlen)
{
   SysRes res;
#  if defined(VGO_darwin) || defined(VGO_freebsd)
   res = VG_(do_syscall6)(__NR___sysctl,
                           (UWord)name, namelen, (UWord)oldp, (UWord)oldlenp, (UWord)newp, newlen);
#  else
   res = VG_(mk_SysRes_Error)(VKI_ENOSYS);
#  endif
   return sr_isError(res) ? -1 : sr_Res(res);
}

It is only called by FreeBSD and Darwin. On other platforms it should never be called and returns an error.

The calls that I see to this function are:
2 for Darwin in void VG_(print_preamble)(Bool logging_to_fd), m_libcprint.c lines 297 and 299, both with a MNULL newp.

3 for FreeBSD, read_and_set_osrel() readelf.c line 1177 and 2 in m_libcproc.c, VG_(sysctlbyname)() lines 1151 and 1155.
Comment 7 Paul Floyd 2021-10-05 20:48:33 UTC
Created attachment 142184 [details]
diff in include directory

This is clean
Comment 8 Paul Floyd 2021-10-06 21:07:27 UTC
Code committed as
commit 7774acbc9c9c4caac2e6e2400635ddde667af6c7