Bug 477630

Summary: Include ucontext.h rather than sys/ucontext.h in Solaris sources
Product: [Developer tools] valgrind Reporter: Jakub Kulik <kulikjak>
Component: generalAssignee: Paul Floyd <pjfloyd>
Status: RESOLVED FIXED    
Severity: normal CC: pjfloyd
Priority: NOR    
Version First Reported In: 3.21.0   
Target Milestone: ---   
Platform: Other   
OS: Linux   
Latest Commit: Version Fixed/Implemented In:
Sentry Crash Report:
Attachments: ucontext patch

Description Jakub Kulik 2023-11-27 17:34:23 UTC 
Created attachment 163547 [details]
ucontext patch

Recently, Oracle Solaris introduced some changes to "ucontext.h" and "sys/ucontext.h" headers, which made short register names on 32bit Solaris no longer available in "sys/ucontext.h", resulting in the following Valgrind build failures:

/builds/valgrind/valgrind-3.21.0/include/vki/vki-solaris.h:1398:17: error: 'EIP' undeclared (first use in this function); did you mean 'EIO'?
 1398 | #define VKI_EIP EIP
      |                 ^~~
m_signals.c:632:74: note: in expansion of macro 'VKI_EIP'
  632 | #  define VG_UCONTEXT_INSTR_PTR(uc)       ((Addr)(uc)->uc_mcontext.gregs[VKI_EIP])
      | 

They are still available when "ucontext.h" is included, which (as I was told by more knowledgeable people internally) is the header that should be included anyway (sys/ucontext.h is not part of the ABI).

And so, in the attached patch, I changed all includes from "sys/ucontext.h" to "ucontext.h" in all Solaris related source files.
(In theory, only a few header files needed this change to build Valgrind, but I don't see any harm in changing all of them to "more correct" one).

I did a test on Illumos/OpenIndiana that changing all "sys/ucontext.h" includes to "ucontext.h" didn't break anything there (as this doesn't affect OpenSolaris distros).


Side note:
Internally, I initially solved it by replacing all short names with long ones:
https://github.com/oracle/solaris-userland/blob/ef7a00bf2fb7f6a16a451d57c477778007aee2ee/components/valgrind/patches/02-registers.patch
but that might not work cross platform - the attached patch should work everywhere.
Comment 1 Paul Floyd 2023-11-27 20:40:55 UTC 
Does this fix issues on more recent versions of Solaris?
Comment 2 Paul Floyd 2023-11-27 21:14:16 UTC 
(In reply to Paul Floyd from comment #1)
> Does this fix issues on more recent versions of Solaris?

Or more precisely, which versions?
Comment 3 Paul Floyd 2023-11-28 07:01:46 UTC 
commit 03180d4403b2b6e849d0a311584ed8b1e6528158
Author: Paul Floyd <pjfloyd@wanadoo.fr>
Date:   Tue Nov 28 08:00:19 2023 +0100

    Bug 477630 - Include ucontext.h rather than sys/ucontext.h in Solaris sources
    
    Patch provided by
       Jakub Kulik kulikjak@gmail.com
Comment 4 Jakub Kulik 2023-11-28 09:51:54 UTC 
Thanks for the merge!

The change in header files that necessitates this patch was done in SRU24 released in August 2020.
Comment 5 Paul Floyd 2023-11-29 18:53:57 UTC 
OK when I have a moment I'll try installing Solaris 11 CBE in a VM.