https://bugs.kde.org/show_bug.cgi?id=433510
--- Comment #4 from Paul Floyd <pjfl...@wanadoo.fr> ---
(In reply to Mark Wielaard from comment #3)
> We really should rename coregrind/m_aspacemgr/aspacemgr-linux.c one day...
>
> This code formatting is really ugly/unreadable:
{ VKI_MAP_STACK }
I'll see what I can do. Just defining this to 0 in FreeBSD should also work,
with a comment to explain that it doesn't really exist.
> - TRACE_SYMTAB("rx_map: avma %#lx size %lu foff %ld\n",
> + TRACE_SYMTAB("rx_map: avma %#lx size %lu foff %lld\n",
^^^ generates warnings
I think that both versions probably generate warnings on either 32bit or 64bit
systems, and that we should use a macro to choose l or ll.
>
> In coregrind/m_debuginfo/storage.c we have:
>
> +#if defined(VGO_freebsd)
> + if (sym->size == 0)
> + sym->size = 1;
> +#endif
>
> Why? Do we not want to ignore zero-sized syms? Are those really size 1?
I don't know why this was done, it predates my work and I can#t see the change
in any git or mercurial repo that I have access to. I'll do some more testing
to see if it is still needed.
>
> coregrind/pub_core_gdbserver.h What is the padding in padding? I see it is
> actually set in coregrind/m_gdbserver/remote-utils.c remote_open. But why?
I don't remember exactly what the problem was, but this is the implicit padding
that is there anyway. The memory gets initialized with brace intialization
leaving the padding uninitialized. This code makes sure that the entire struct
gets initialized, even the unused last 4 bytes.
> The logic and the defines feel swapped in VG_(is32on64)(void). But I might
> misunderstand why the check for that file is done.
It's very hard to tell when a 32bit process is running on an amd64 kernel (for
instance, all sysctls behave like a true 32bit system). When compiling the
amd64 binary it returns false - that can only ever be 64on64. When compiling
for x86 that is either on an x86 system or and amd64 system building both amd64
and x86. I'm using the link loader to tell them apart. This isn't totally bomb
proof. Someone could try to run an x86 built Valgrind on amd64 or add a
/libexec/ld-elf32.so.1 on an x86 system, but I'm not going to try to handle
user foolishness like that.
>
> /* Number of file descriptors that Valgrind tries to reserve for
> its own use - just a small constant. */
> +#if defined(VGO_freebsd)
> +#define N_RESERVED_FDS (20)
> +#else
> #define N_RESERVED_FDS (12)
> +#endif
>
> Why does FreeBSD need 8 more?
Again that predates my work and I'll recheck.
> I don't understand what SIGSYS is/does. This patch adds/clears it
> unconditionally, not just for freebsd. Is that correct?
SIGSYS gets sent for a non-existent syscall. I'll write a small test and see if
it's needed on Linux and FreeBSD.
> Why this in coregrind/m_main.c
>
> +void *memcpy(void *dest, const void *src, size_t n);
> +void *memcpy(void *dest, const void *src, size_t n) {
> + return VG_(memcpy)(dest, src, n);
> +}
> +void* memmove(void *dest, const void *src, SizeT n);
> +void* memmove(void *dest, const void *src, SizeT n) {
> + return VG_(memmove)(dest,src,n);
> +}
> +void* memset(void *s, int c, SizeT n);
> +void* memset(void *s, int c, SizeT n) {
> + return VG_(memset)(s,c,n);
> +}
That's clang's fault. clang can convert things like
structA = {0};
and
structA = structB;
into memset and memcoy calls, so FreeBSD needs them in the tool exe.
> aha, this is where the realloc support is added. Dont forget to close bug
> #407589.
>
> Was this deliberate?
>
> diff --git a/coregrind/m_scheduler/sema.c b/coregrind/m_scheduler/sema.c
> index 61e10dcf0..5954cdf8f 100644
> --- a/coregrind/m_scheduler/sema.c
> +++ b/coregrind/m_scheduler/sema.c
> @@ -108,8 +108,8 @@ void ML_(sema_down)( vg_sema_t *sema, Bool as_LL )
> INNER_REQUEST(ANNOTATE_RWLOCK_ACQUIRED(sema, /*is_w*/1));
>
> if (ret != 1)
> - VG_(debugLog)(0, "scheduler",
> - "VG_(sema_down): read returned %d\n", ret);
> + VG_(debugLog)(1, "scheduler",
> + "ML_(sema_down): read returned %d\n", ret);
Yes, that was deliberate, it was causing too much noise with debug 0 on
FreeBSD.
> This change:
>
> @@ -2459,9 +2528,9 @@ void async_signalhandler ( Int sigNo,
> info->si_code = sanitize_si_code(info->si_code);
>
> if (VG_(clo_trace_signals))
> - VG_(dmsg)("async signal handler: signal=%d, tid=%u, si_code=%d, "
> + VG_(dmsg)("async signal handler: signal=%d, vgtid=%u, tid=%u,
> si_code=%d, "
> "exitreason %s\n",
> - sigNo, tid, info->si_code,
> + sigNo, VG_(gettid)(), tid, info->si_code,
> VG_(name_of_VgSchedReturnCode)(tst->exitreason));
>
> /* See similar logic in VG_(poll_signals). */
>
> Causes this warning:
I'll fix that.
--
You are receiving this mail because:
You are watching all bug changes.
