Re: [PATCH] Add -D_FORTIFY_SOURCE=2 to CFLAGS if possible.
On 09 Feb 2017 21:13, Mark Wielaard wrote:
> +# See if we can add -D_FORTIFY_SOURCE=2. Don't do it if it is already
> +# (differently) defined or if it generates warnings/errors because we
> +# don't use the right optimisation level (string.h will warn about that).
> +AC_MSG_CHECKING([whether to add -D_FORTIFY_SOURCE=2 to CFLAGS])
-D flags should be in CPPFLAGS ...
> +case "$CFLAGS" in
> + *-D_FORTIFY_SOURCE=2*)
> +AC_MSG_RESULT([no, already there])
> +;;
> + *)
> +save_CFLAGS="$CFLAGS"
> +CFLAGS="-D_FORTIFY_SOURCE=2 -Werror $CFLAGS"
> +AC_COMPILE_IFELSE([AC_LANG_SOURCE([[
> + #include
> + int main() { return 0; }
> +]])], [ AC_MSG_RESULT([yes])
> +CFLAGS="-D_FORTIFY_SOURCE=2 $save_CFLAGS" ],
> + [ AC_MSG_RESULT([no])
> +CFLAGS="$save_CFLAGS"])
> + ;;
why not AC_PREPROC_IFELSE and check if _FORTIFY_SOURCE is defined ?
-mike
signature.asc
Description: Digital signature
Re: [PATCH] Check for existence of asprintf and vasprintf
On 22 Feb 2017 13:50, Ulf Hermann wrote: > Add replacements to libeu.a if they don't exist. Include system.h > and link against libeu.a where they are used. these portability replacements are starting to get out of hand -mike signature.asc Description: Digital signature
Re: [PATCH] Check for existence of asprintf and vasprintf
On 22 Feb 2017 17:01, Ulf Hermann wrote: > > these portability replacements are starting to get out of hand > > To what extent should elfutils be portable to non-GNU systems? My goal here > is to port it to windows while minimizing the amount of external dependencies > I have to add. The functions I have replaced so far are so trivial that I > didn't consider it worthwhile to import things from gnulib in order to > provide them. imo, elfutils shouldn't be growing these fallback implementations itself. if you want to do this stuff, use gnulib instead. then there is no ifdef hell in the source files, and you don't have to worry about testing whether the ifdef's are correct because gnulib did it all for you. -mike signature.asc Description: Digital signature
Re: [PATCH] Check for existence of asprintf and vasprintf
On 22 Feb 2017 17:45, Ulf Hermann wrote:
> > imo, elfutils shouldn't be growing these fallback implementations itself.
> > if you want to do this stuff, use gnulib instead.
> >
> > then there is no ifdef hell in the source files, and you don't have to
> > worry about testing whether the ifdef's are correct because gnulib did
> > it all for you.
>
> OK, so I won't post any more patches with replacement functions. I don't like
> the idea of using gnulib because the gnulib implementations are generally
> more complex and in order to maintain them I would have to track gnulib in
> addition to elfutils.
sorry, but i don't know what you're talking about. you don't read the
gnulib code/modules directly, you just run gnulib-tool and tell it which
modules to import. it does all the rest for you.
you want asprintf ? then add it to the list.
modules=(
asprintf
glob
vasprintf
...whatever else you want...
)
gnulib-tool --import "${modules[@]}"
stick that in an autogen.sh script.
> Also I would have to review the licenses for each import. This is not
> something I'm excited to do for a few 3-line functions. But OK, I will
> consider it.
gnulib-tool has a --lgpl=[...] flag so you can automatically abort if
the desired license compatibility level isn't met. so you don't have
to directly review every module if it isn't aborting.
-mike
signature.asc
Description: Digital signature
Re: [PATCH] Check for existence of asprintf and vasprintf
On 22 Feb 2017 18:43, Ulf Hermann wrote:
> > sorry, but i don't know what you're talking about. you don't read the
> > gnulib code/modules directly, you just run gnulib-tool and tell it which
> > modules to import. it does all the rest for you.
> >
> > you want asprintf ? then add it to the list.
> > modules=(
> > asprintf
> > glob
> > vasprintf
> > ...whatever else you want...
> > )
> > gnulib-tool --import "${modules[@]}"
>
> I see. Some of the functions are not available from gnulib, though. In
> particular, GNU-style basename abd GNU-style strerror_r. There might be more.
you're probably looking at the raw module list.
you'll want to look at the full documentation instead:
https://www.gnu.org/software/gnulib/MODULES.html
basename() is in the dirname module
you're correct that GNU strerror_r is not handled by gnulib.
that doesn't look like it's too hard to deal with, but it is
something that'd have to be considered.
-mike
signature.asc
Description: Digital signature
Re: [PATCH] Check for existence of asprintf and vasprintf
On 23 Feb 2017 10:39, Ulf Hermann wrote: > > basename() is in the dirname module > > That's the POSIX variant. We're using the GNU variant everywhere looking closer, it's not either. POSIX doesn't guarantee that the input is not modified, while GNU guarantees that it is left alone. the gnulib one makes the same guarantee as GNU, but it also returns a new buffer. seems like getting a new basename module into gnulib wouldn't be *too* difficult to pull off. > and the GNU variant is a whopping two lines of code: > > char *base = strrchr(path, '/'); > return base ? base + 1 : (char *)path; and we get straight to an example of why your solutions don't scale. your replacement is wrong. and ironically, it's wrong on Windows, which is the whole point of your work. '/' is not the path sep used on every system out there. it also does not properly handle systems (such as Windows) that have filesystem prefixes like C:\. > > you're correct that GNU strerror_r is not handled by gnulib. > > that doesn't look like it's too hard to deal with, but it is > > something that'd have to be considered. > > We're using strerror_r in exactly one place and my proposal is to just return > a fixed string if we hit that on platforms where GNU strerror_r is not > available. i don't have an opinion on that -mike signature.asc Description: Digital signature
Re: [PATCH] Check for existence of asprintf and vasprintf
On 24 Feb 2017 10:42, Ulf Hermann wrote: > >> and the GNU variant is a whopping two lines of code: > >> > >> char *base = strrchr(path, '/'); > >> return base ? base + 1 : (char *)path; > > > > and we get straight to an example of why your solutions don't scale. > > your replacement is wrong. and ironically, it's wrong on Windows, > > which is the whole point of your work. '/' is not the path sep used > > on every system out there. it also does not properly handle systems > > (such as Windows) that have filesystem prefixes like C:\. > > Both basename variants' documentations only talk about '/'. This is not > the place where we should handle windows directory separators. umm, sure it is. if dirname/basename don't handle the OS separator, then where do you think it should be handled ? every place that calls basename ? GNU/POSIX not talking about it isn't terribly relevant -- they don't care about systems like Windows that have diff path name semantics. although it's a moot point if you move everything to gnulib as they handle the issue properly. -mike signature.asc Description: Digital signature
