[PATCH] D85870: sanitizer_common: Introduce internal_madvise and start using it.

[Rainer Orth via Phabricator via llvm-commits]

ro added inline comments.


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_solaris.cpp:79
+// is defined as g++ does on Solaris.
+extern "C" int madvise(caddr_t, size_t, int);
+
----------------
pcc wrote:
> ro wrote:
> > pcc wrote:
> > > ro wrote:
> > > > I don't know why but you reintroduced an older/broken version of the `madvise` declaration here.  The one you removed from `sanitizer_posix_libcdep.cpp` had the first arg as `void *` for a reason.
> > > Oh, I thought that was a mistake and that madvise was supposed to be defined like this on Solaris (according to the comment that I removed on sanitizer_posix_libcdep.cpp lines 70-72, and man pages that I found online, e.g. https://docs.oracle.com/cd/E36784_01/html/E36874/madvise-3c.html). If it's actually defined to take `void *` on Solaris then I can fix it.
> > On the contrary: I had provided a code snippet using `void *', carefully tested on both Solaris 11.4 and Illumos in D84046.  Unfortunately, the author of that patch chose to make the same change as you did, silently and at the last minute and breaking the Solaris build ;-(
> > 
> > I had to restore the build in D84388 and thus was particularly dismayed to see the same change going in again for no apparent reason and without testing.
> > 
> > Strangely, 1-stage builds with gcc 9 or 10 still do work, as can be seen on the Solaris buildbots, but when I tried a 2-stage build last night, it failed again as it had before:
> > ```
> > /vol/llvm/src/llvm-project/dist/compiler-rt/lib/sanitizer_common/sanitizer_solaris.cpp:79:16: error: conflicting types for 'madvise'
> > extern "C" int madvise(caddr_t, size_t, int);
> >                ^
> > /usr/include/sys/mman.h:232:12: note: previous declaration is here
> > extern int madvise(void *, size_t, int);
> >            ^
> > 1 error generated.
> > ```
> > The man page you link is from Solaris 11.2, about 8 years ago.  The signature has been changed in 11.4 exactly to avoid unnecessary incompatibilities with other systems.
> > 
> > So yes, please restore the `void *` version.
> Okay, I sent D86166 to fix this.
> 
> I have a followup question which I will raise there, since I think the comment really needs to say *why* the declaration is this way, otherwise someone else may come along and "fix" it again.
> Okay, I sent D86166 to fix this.
> 
> I have a followup question which I will raise there, since I think the comment really needs to say *why* the declaration is this way, otherwise someone else may come along and "fix" it again.

But why on earth do people insist on changing/breaking working code on platforms that by their own admission they know nothing about instead of first asking someone that does for clarification?  That's completely beyond me!

I do run Solaris buildbots for a reason.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85870/new/

https://reviews.llvm.org/D85870