Bug 433439

Created attachment 141834 [details]
diff in include directory

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.

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.

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.

(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.

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.

Created attachment 142184 [details]
diff in include directory

This is clean

Code committed as
commit 7774acbc9c9c4caac2e6e2400635ddde667af6c7