Re: [PATCH,V3 08/15] unwinder: generate backtrace using SFrame format

[Indu Bhagat <indu.bhagat@oracle.com>]

(Adding Weimin Pan who is the author of the patch)

On 10/30/22 07:03, Mike Frysinger wrote:
> On 30 Oct 2022 00:44, Indu Bhagat via Binutils wrote:
>> --- /dev/null
>> +++ b/config/sframe.m4
>> @@ -0,0 +1,16 @@
>> +# SFRAME_CHECK_AS_SFRAME
>> +# ----------------------
>> +# Check whether the assembler supports generation of SFrame
>> +# unwind information.
>> +#
>> +# Defines:
>> +# ac_cv_have_sframe
>> +
> you should be using `dnl` for comments in m4 files so they aren't copied
> into the generated output.
> 
>> +AC_DEFUN([SFRAME_CHECK_AS_SFRAME],[
> space after the ,
> 
>> +  ac_save_CFLAGS="$CFLAGS"
>> +  CFLAGS="$CFLAGS -Wa,--gsframe"
>> +  AC_MSG_CHECKING([for as that supports --gsframe])
>> +  AC_TRY_COMPILE([], [return 0;], [ac_cv_have_sframe=yes], [ac_cv_have_sframe=no])
>> +  AC_MSG_RESULT($ac_cv_have_sframe)
>> +  CFLAGS="$ac_save_CFLAGS"
>> +])
> you call it "ac_cv_have_sframe" which implies it's an autoconf cached var,
> but you aren't actually using the AC_CACHE_CHECK macro.
> 

I will use the AC_CACHE_CHECK macro in V4. Also changed the prefix to 
"sframe_cv_" instead.

> i'm guessing this isn't actually coming from autoconf, or will be merged
> there, so shouldn't this be using a "gcc_cv_" prefix instead ?  i'm not
> sure what the policy is on config/ when it comes to home-grown cache vars.
> 
> similarly, should the macro name lacks scoping ...
> 

Can you please clarify what is meant by "should the macro name lack 
scoping.." in the current context ?

>> --- a/libsframe/configure.ac
>> +++ b/libsframe/configure.ac
>>
>>   COMPAT_DEJAGNU=$ac_cv_dejagnu_compat
>>   AC_SUBST(COMPAT_DEJAGNU)
>>   
>> +dnl The libsframebt library needs to be built with SFrame info.
>> +dnl If the build assembler is not capable of generate SFrame then
>> +dnl the library is not built.
>> +
>> +SFRAME_CHECK_AS_SFRAME
>> +AM_CONDITIONAL([HAVE_SFRAME_AS], [test "x$ac_cv_have_sframe" = "xyes"])
> hmm, is this macro only used by libsframe/ ?  if no one else is going to use
> this macro, config/ isn't the right place for it.  you should put it into
> libsframe/acinclude.m4 instead.
> 

Yes, at this time the macro is only being used in libsframe. OK, I will 
move it to libsframe/acinclude.m4 in V4.

Thanks