Re: ASH: why ash_ptr_to_globals_misc is declared as const pointer

[Mike Frysinger]

On 30 Jan 2018 04:10, Denys Vlasenko wrote:
> On Fri, Jan 26, 2018 at 8:39 PM, Yunlian Jiang  wrote:
> > I will use a similar issue in lineedit.c to explain the problem. (it is
> > easier to reproduce).
> > The problem is that
> > clang thinks lineedit_ptr_to_statics is a constant pointer, so the pointer
> > should be unchanged. As a result, inside a function, it
> > loads the value once, and it can use it as many as it want without worrying
> > anthing changes.
> > In the macro
> >
> >  #define INIT_S() do { \
> > (*(struct lineedit_statics**)_ptr_to_statics) =
> > xzalloc(sizeof(S)); \
> > barrier(); \
> > cmdedit_termw = 80; \
> > IF_USERNAME_OR_HOMEDIR(home_pwd_buf = (char*)null_str;) \
> > IF_FEATURE_EDITING_VI(delptr = delbuf;) \
> > } while (0)
> >
> >  (*(struct lineedit_statics**)_ptr_to_statics) =
> > xzalloc(sizeof(S));
> > actually changes value of the pointer (from 0x0 to the return value of
> > xzalloc). But clang did not realize that, otherwise, it
> > should omit a compiler time error, so clang thinks it has nothing to do with
> > lineedit_ptr_to_statics
> >
> > in the statement
> > IF_FEATURE_EDITING_VI(delptr = delbuf;), clang still assumes the value of
> > the pointer is '0x0', so the segfaults happens.
> 
> How clang knows that pointer was NULL? It's an extern, clang can't
> infer its value
> because it doesn't see it, right?

i think he means is that clang loaded the pointer before the assignment/barrier
and it wasn't reloaded or delayed after the barrier, so it started out NULL and
stayed that way.

but i'm guessing ... the .s output would help as you suggested.
-mike


signature.asc
Description: Digital signature
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: ASH: why ash_ptr_to_globals_misc is declared as const pointer

[Denys Vlasenko]

On Fri, Jan 26, 2018 at 8:39 PM, Yunlian Jiang  wrote:
> I will use a similar issue in lineedit.c to explain the problem. (it is
> easier to reproduce).
> The problem is that
> clang thinks lineedit_ptr_to_statics is a constant pointer, so the pointer
> should be unchanged. As a result, inside a function, it
> loads the value once, and it can use it as many as it want without worrying
> anthing changes.
> In the macro
>
>  #define INIT_S() do { \
> (*(struct lineedit_statics**)_ptr_to_statics) =
> xzalloc(sizeof(S)); \
> barrier(); \
> cmdedit_termw = 80; \
> IF_USERNAME_OR_HOMEDIR(home_pwd_buf = (char*)null_str;) \
> IF_FEATURE_EDITING_VI(delptr = delbuf;) \
> } while (0)
>
>  (*(struct lineedit_statics**)_ptr_to_statics) =
> xzalloc(sizeof(S));
> actually changes value of the pointer (from 0x0 to the return value of
> xzalloc). But clang did not realize that, otherwise, it
> should omit a compiler time error, so clang thinks it has nothing to do with
> lineedit_ptr_to_statics
>
> in the statement
> IF_FEATURE_EDITING_VI(delptr = delbuf;), clang still assumes the value of
> the pointer is '0x0', so the segfaults happens.

How clang knows that pointer was NULL? It's an extern, clang can't
infer its value
because it doesn't see it, right?

Can you post "make libbb/lineedit.s" result?

Can you experiment with better barriers?
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: ASH: why ash_ptr_to_globals_misc is declared as const pointer

[Mike Frysinger]

On 28 Jan 2018 20:41, Denys Vlasenko wrote:
> On Thu, Jan 18, 2018 at 10:48 PM, Yunlian Jiang  wrote:
> > Hi,
> >I tried to build busybox with clang and use it to create recovery image
> > for ChromeOS.
> > It fails to recover an arm based ChromeBook.
> >I digged a little bit.
> >Below patch makes it work. My question is.
> >the  ash_ptr_to_globals_misc, ash_ptr_to_globals_memstack and
> > ash_ptr_to_globals_var
> > are defined in ash_ptr_hack.c as normal pointers. But in ash.c, they are
> > declared as const
> > pointers. What is the benefit of doing that?
> >
> >Thanks
> > --- busybox-1.27.2/shell/ash.c
> > +++ busybox-1.27.2/shell/ash.c
> > @@ -378,7 +378,11 @@ struct globals_misc {
> >  #endif
> > pid_t backgndpid;/* pid of last background process */
> >  };
> > +#ifndef GCC_COMBINE
> > +extern struct globals_misc * ash_ptr_to_globals_misc;
> > +#else
> >  extern struct globals_misc *const ash_ptr_to_globals_misc;
> > +#endif
> >  #define G_misc (*ash_ptr_to_globals_misc)
> >  #define exitstatus(G_misc.exitstatus )
> >  #define back_exitstatus   (G_misc.back_exitstatus )
> > @@ -1431,7 +1435,11 @@ struct globals_memstack {
> > size_t g_stacknleft; // = MINSIZE;
> > struct stack_block stackbase;
> >  };
> > +#ifndef GCC_COMBINE
> > +extern struct globals_memstack * ash_ptr_to_globals_memstack;
> > +#else
> >  extern struct globals_memstack *const ash_ptr_to_globals_memstack;
> > +#endif
> 
> 
> I committed a bit different fix, a-la

you've only updated ash.c though ... all of the users of this style
can run into the same issue.  i see:
test_ptr_to_statics
lineedit_ptr_to_statics
ptr_to_globals

might be a good time to try to unify this stuff with some helper macros ?

#ifndef GCC_COMBINE
#define G_DECL_STORAGE(type, name) type *name
#else
#define G_DECL_STORAGE(type, name) type *const name __attribute__ ((__section__ 
(".data")))
#endif
#define G_DECL(type, name) extern type *const name

i wonder if the barrier could be changed in a way to make clang load after the
assignment though.
-mike


signature.asc
Description: Digital signature
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: ASH: why ash_ptr_to_globals_misc is declared as const pointer

[Denys Vlasenko]

On Thu, Jan 18, 2018 at 10:48 PM, Yunlian Jiang  wrote:
> Hi,
>I tried to build busybox with clang and use it to create recovery image
> for ChromeOS.
> It fails to recover an arm based ChromeBook.
>I digged a little bit.
>Below patch makes it work. My question is.
>the  ash_ptr_to_globals_misc, ash_ptr_to_globals_memstack and
> ash_ptr_to_globals_var
> are defined in ash_ptr_hack.c as normal pointers. But in ash.c, they are
> declared as const
> pointers. What is the benefit of doing that?
>
>Thanks
> --- busybox-1.27.2/shell/ash.c
> +++ busybox-1.27.2/shell/ash.c
> @@ -378,7 +378,11 @@ struct globals_misc {
>  #endif
> pid_t backgndpid;/* pid of last background process */
>  };
> +#ifndef GCC_COMBINE
> +extern struct globals_misc * ash_ptr_to_globals_misc;
> +#else
>  extern struct globals_misc *const ash_ptr_to_globals_misc;
> +#endif
>  #define G_misc (*ash_ptr_to_globals_misc)
>  #define exitstatus(G_misc.exitstatus )
>  #define back_exitstatus   (G_misc.back_exitstatus )
> @@ -1431,7 +1435,11 @@ struct globals_memstack {
> size_t g_stacknleft; // = MINSIZE;
> struct stack_block stackbase;
>  };
> +#ifndef GCC_COMBINE
> +extern struct globals_memstack * ash_ptr_to_globals_memstack;
> +#else
>  extern struct globals_memstack *const ash_ptr_to_globals_memstack;
> +#endif


I committed a bit different fix, a-la


+++ b/shell/ash.c
@@ -263,6 +263,18 @@ typedef long arith_t;
 # error "Do not even bother, ash will not run on NOMMU machine"
 #endif

+/* We use a trick to have more optimized code (fewer pointer reloads):
+ *  ash.c:   extern struct globals *const ash_ptr_to_globals;
+ *  ash_ptr_hack.c: struct globals *ash_ptr_to_globals;
+ * This way, compiler in ash.c knows the pointer can not change.
+ *
+ * However, this may break on weird arches or toolchains. In this case,
+ * set "-DBB_GLOBAL_CONST=''" in CONFIG_EXTRA_CFLAGS to disable this
optimization.
+ */
+#ifndef BB_GLOBAL_CONST
+# define BB_GLOBAL_CONST const
+#endif
+

 /*  Hash table sizes. Configurable. */

@@ -400,7 +412,7 @@ struct globals_misc {
 #endif
pid_t backgndpid;/* pid of last background process */
 };
-extern struct globals_misc *const ash_ptr_to_globals_misc;
+extern struct globals_misc *BB_GLOBAL_CONST ash_ptr_to_globals_misc;
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: ASH: why ash_ptr_to_globals_misc is declared as const pointer

[Mike Frysinger]

On 26 Jan 2018 11:39, Yunlian Jiang wrote:
> I will use a similar issue in lineedit.c to explain the problem. (it is
> easier to reproduce).
> The problem is that
> clang thinks lineedit_ptr_to_statics is a constant pointer, so the pointer
> should be unchanged. As a result, inside a function, it
> loads the value once, and it can use it as many as it want without worrying
> anthing changes.
> In the macro
> 
>  #define INIT_S() do { \
> (*(struct lineedit_statics**)_ptr_to_statics) =
> xzalloc(sizeof(S)); \
> barrier(); \
> cmdedit_termw = 80; \
> IF_USERNAME_OR_HOMEDIR(home_pwd_buf = (char*)null_str;) \
> IF_FEATURE_EDITING_VI(delptr = delbuf;) \
> } while (0)
> 
>  (*(struct lineedit_statics**)_ptr_to_statics) =
> xzalloc(sizeof(S));
> actually changes value of the pointer (from 0x0 to the return value of
> xzalloc). But clang did not realize that, otherwise, it
> should omit a compiler time error, so clang thinks it has nothing to do
> with lineedit_ptr_to_statics
> 
> in the statement
> IF_FEATURE_EDITING_VI(delptr = delbuf;), clang still assumes the value of
> the pointer is '0x0', so the segfaults happens.
> 
> 
> I think compiler has the right to assume the pointer is unchanged because
> it is declared as const.

thanks, that analysis looks good.  we've been using barrier() to trick
compilers into doing the load after we init it (which i'm pretty sure
is the point of commit 574f2f43948bb21d6e4187936ba5a5afccba25f6 [1]).
the question is how can we trick clang into delaying the load :).
-mike

[1] 
https://git.busybox.net/busybox/commit/?id=574f2f43948bb21d6e4187936ba5a5afccba25f6


signature.asc
Description: Digital signature
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: ASH: why ash_ptr_to_globals_misc is declared as const pointer

[Mike Frysinger]

On 22 Jan 2018 22:59, Ralf Friedl wrote:
> Mike Frysinger wrote:
> > On 20 Jan 2018 19:03, Ralf Friedl wrote:
> >> Mike Frysinger wrote:
> >>> the pointer itself is the thing that is const, not the memory it points 
> >>> to.
> >>> this lets the compiler optimize the loads by generating relocations via 
> >>> the
> >>> pointer ... there's the fixup at the initial load time, but after that, 
> >>> it's
> >>> just offsets to a constant memory location.  but removing the const 
> >>> markings,
> >>> the compiler has to reload the pointer everytime.
> >> Actually, why does the comipler have to reload the pointer? In your
> >> example, there is no other function called between accesses to the
> >> pointer that might change the pointer, and anti-aliasing should let the
> >> compiler know that the memory modified through the pointer can't be the
> >> pointer itself because of the type difference.
> > when i say reload, i don't mean in a single function.  i agree as you say 
> > that
> > the compiler has no need based on its memory model.  i meant that on every 
> > call,
> > the pointer has to be loaded because the compiler cannot know that it hasn't
> > changed.  by having the pointer be const, gcc knows that it can be referred 
> > to
> > directly.
> >
> > it's a pretty esoteric optimization/hack, but busybox has a lot of these 
> > things,
> > and when you add them up together, it makes for a difference.  i'm not sure 
> > how
> > this can be written portably though since we're pushing the limits of the
> > compiler :/.
> You say you don't mean in a single function, but your example with
> raise_exception shows that it does happen within a single function, so
> the question is, why does the compiler not recognize it?
>
> Maybe because anti-aliasing is not enabled? I tried to compile busybox
> with -O3 instead of -O2, and I get at least some warnings, and together
> with -Wall they abort the compilation. I had to add
> "-Wno-error=format-overflow -Wno-error=format-truncation" to
> EXTRA_CFLAGS for gcc 7.2.1. One could argue whether gcc should warn for
> snprintf (in contrast to sprintf), but at the moment it does, at least
> unless the return value is used.

my guess is because the code has a memory barrier between the two
stores, but i haven't thought too hard about it.

no.const/ :
static void
raise_exception(int e)
   0:   48 83 ec 08 sub$0x8,%rsp
{
suppress_int++;
   4:   48 8b 15 00 00 00 00mov0x0(%rip),%rdx# b 

7: R_X86_64_PC32ash_ptr_to_globals_misc-0x4
   b:   8b 42 38mov0x38(%rdx),%eax
   e:   ff c0   inc%eax
  10:   89 42 38mov%eax,0x38(%rdx)

barrier();

exception_type = e;
>>> extra load <<<
>  13:  48 8b 05 00 00 00 00mov0x0(%rip),%rax# 1a 
> 
   16: R_X86_64_PC32   ash_ptr_to_globals_misc-0x4
  1f:   40 88 78 3f mov%dil,0x3f(%rax)

longjmp(exception_handler->loc, 1);
  1a:   be 01 00 00 00  mov$0x1,%esi
  23:   48 8b 78 30 mov0x30(%rax),%rdi
  27:   e8 00 00 00 00  callq  2c 
28: R_X86_64_PC32   __longjmp_chk-0x4
}
-mike


signature.asc
Description: Digital signature
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: ASH: why ash_ptr_to_globals_misc is declared as const pointer

[Mike Frysinger]

On 23 Jan 2018 07:11, Ralf Friedl wrote:
> Mike Frysinger schrieb:
> > On 22 Jan 2018 22:54, Ralf Friedl wrote:
> >> Mike Frysinger schrieb:
> >>> On 22 Jan 2018 09:23, Yunlian Jiang wrote:
>  On Fri, Jan 19, 2018 at 3:32 PM, Mike Frysinger wrote:
> > is it that busybox is crashing ?  is clang/lld placing this pointer in
> > const
> > memory (even though we have section(".data")) ?  or is it generating an
> > abort
> > when it reaches what it thinks is an undefined situation (like trying to
> > write
> > to a const memory location in INIT_G_var()) ?
> >
>  The busybox is crashing. On arm boards, the command line
>  'busybox ash' gave me a segmentation fault.
> 
>  In the ash_ptr_hack.c, it sets the attribute  __attribute__ (section
>  (".data")) when
>  'GCC_COMBINE' is defined.  But clang does not have this MACRO defined.
> >>> i don't think GCC_COMBINE is relevant, so just forget about it.  
> >>> ash_ptr_hack.c
> >>> does:
> >>> struct globals_misc *ash_ptr_to_globals_misc;
> >>> that means that pointer should end up the .data/writable memory by 
> >>> default.
> >>>
> >>> the fact that another compilation unit declares it as const isn't terribly
> >>> relevant ... the actual storage should still be writable.  you should 
> >>> check
> >>> that is happening (or if it's ending up somewhere else).  we've seen llvm
> >>> incorrectly propagate attributes to storage before based on decls seen in
> >>> other modules.
> >> GCC_COMBINE is relevant because it changes the definitions aof the
> >> pointers. without GCC_COMBINE, they are defined as normal variables, and
> >> every compiler should be able to compile them. With GCC_COMBINE they are
> >> defined as const but with section ".data", and a compiler that doesn't
> >> understand that will fail. in busybox GCC_COMBINE is only set in
> >> scripts/Makefile.IMA, together with -combine and -fwhole-program. The
> >> first line of this script says "This is completely unsupported." A
> >> compiler that doesn't support -combine and -fwhole-program doesn't need
> >> to define GCC_COMBINE and won't have a problem with the code. If the
> >> compiler supports the concept of -fwhole-program, the trick to use a
> >> different definition and declaration in different files no longer works.
> >> But maybe then the compiler is smart enough to realize that there are no
> >> other assignments to the pointers so it can omit the reload. If not,
> >> something like the ".data" section is necessary, and if the compiler has
> >> -fwhole-program but no ".data", then this can't be used.
> > GCC_COMBINE isn't relevant to this thread because we aren't building with
> > gcc, nor doing are we doing an IMA build.  hence GCC_COMBINE is never 
> > defined
> > and it has no impact on Yunlian's setup.  we're only interested in the clang
> > behavior here.
> But if he doesn't define GCC_COMBINE, whether from from the IMA Makefile 
> or from somewhere else, the definition of the pointer variables should 
> place them in writable space and he shouldn't have the problem he is 
> reporting?

that's why he's reporting a bug and this thread exists and i'm asking for more
info
-mike


signature.asc
Description: Digital signature
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: ASH: why ash_ptr_to_globals_misc is declared as const pointer

[Ralf Friedl]

Mike Frysinger schrieb:

On 22 Jan 2018 22:54, Ralf Friedl wrote:

Mike Frysinger schrieb:

On 22 Jan 2018 09:23, Yunlian Jiang wrote:

On Fri, Jan 19, 2018 at 3:32 PM, Mike Frysinger wrote:

is it that busybox is crashing ?  is clang/lld placing this pointer in
const
memory (even though we have section(".data")) ?  or is it generating an
abort
when it reaches what it thinks is an undefined situation (like trying to
write
to a const memory location in INIT_G_var()) ?


The busybox is crashing. On arm boards, the command line
'busybox ash' gave me a segmentation fault.

In the ash_ptr_hack.c, it sets the attribute  __attribute__ (section
(".data")) when
'GCC_COMBINE' is defined.  But clang does not have this MACRO defined.

i don't think GCC_COMBINE is relevant, so just forget about it.  ash_ptr_hack.c
does:
struct globals_misc *ash_ptr_to_globals_misc;
that means that pointer should end up the .data/writable memory by default.

the fact that another compilation unit declares it as const isn't terribly
relevant ... the actual storage should still be writable.  you should check
that is happening (or if it's ending up somewhere else).  we've seen llvm
incorrectly propagate attributes to storage before based on decls seen in
other modules.

GCC_COMBINE is relevant because it changes the definitions aof the
pointers. without GCC_COMBINE, they are defined as normal variables, and
every compiler should be able to compile them. With GCC_COMBINE they are
defined as const but with section ".data", and a compiler that doesn't
understand that will fail. in busybox GCC_COMBINE is only set in
scripts/Makefile.IMA, together with -combine and -fwhole-program. The
first line of this script says "This is completely unsupported." A
compiler that doesn't support -combine and -fwhole-program doesn't need
to define GCC_COMBINE and won't have a problem with the code. If the
compiler supports the concept of -fwhole-program, the trick to use a
different definition and declaration in different files no longer works.
But maybe then the compiler is smart enough to realize that there are no
other assignments to the pointers so it can omit the reload. If not,
something like the ".data" section is necessary, and if the compiler has
-fwhole-program but no ".data", then this can't be used.

GCC_COMBINE isn't relevant to this thread because we aren't building with
gcc, nor doing are we doing an IMA build.  hence GCC_COMBINE is never defined
and it has no impact on Yunlian's setup.  we're only interested in the clang
behavior here.
But if he doesn't define GCC_COMBINE, whether from from the IMA Makefile 
or from somewhere else, the definition of the pointer variables should 
place them in writable space and he shouldn't have the problem he is 
reporting?


___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: ASH: why ash_ptr_to_globals_misc is declared as const pointer

[Mike Frysinger]

On 22 Jan 2018 22:54, Ralf Friedl wrote:
> Mike Frysinger schrieb:
> > On 22 Jan 2018 09:23, Yunlian Jiang wrote:
> >> On Fri, Jan 19, 2018 at 3:32 PM, Mike Frysinger wrote:
> >>> is it that busybox is crashing ?  is clang/lld placing this pointer in
> >>> const
> >>> memory (even though we have section(".data")) ?  or is it generating an
> >>> abort
> >>> when it reaches what it thinks is an undefined situation (like trying to
> >>> write
> >>> to a const memory location in INIT_G_var()) ?
> >>>
> >> The busybox is crashing. On arm boards, the command line
> >> 'busybox ash' gave me a segmentation fault.
> >>
> >> In the ash_ptr_hack.c, it sets the attribute  __attribute__ (section
> >> (".data")) when
> >> 'GCC_COMBINE' is defined.  But clang does not have this MACRO defined.
> > i don't think GCC_COMBINE is relevant, so just forget about it.  
> > ash_ptr_hack.c
> > does:
> >struct globals_misc *ash_ptr_to_globals_misc;
> > that means that pointer should end up the .data/writable memory by default.
> >
> > the fact that another compilation unit declares it as const isn't terribly
> > relevant ... the actual storage should still be writable.  you should check
> > that is happening (or if it's ending up somewhere else).  we've seen llvm
> > incorrectly propagate attributes to storage before based on decls seen in
> > other modules.
> GCC_COMBINE is relevant because it changes the definitions aof the 
> pointers. without GCC_COMBINE, they are defined as normal variables, and 
> every compiler should be able to compile them. With GCC_COMBINE they are 
> defined as const but with section ".data", and a compiler that doesn't 
> understand that will fail. in busybox GCC_COMBINE is only set in 
> scripts/Makefile.IMA, together with -combine and -fwhole-program. The 
> first line of this script says "This is completely unsupported." A 
> compiler that doesn't support -combine and -fwhole-program doesn't need 
> to define GCC_COMBINE and won't have a problem with the code. If the 
> compiler supports the concept of -fwhole-program, the trick to use a 
> different definition and declaration in different files no longer works. 
> But maybe then the compiler is smart enough to realize that there are no 
> other assignments to the pointers so it can omit the reload. If not, 
> something like the ".data" section is necessary, and if the compiler has 
> -fwhole-program but no ".data", then this can't be used.

GCC_COMBINE isn't relevant to this thread because we aren't building with
gcc, nor doing are we doing an IMA build.  hence GCC_COMBINE is never defined
and it has no impact on Yunlian's setup.  we're only interested in the clang
behavior here.
-mike


signature.asc
Description: Digital signature
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: ASH: why ash_ptr_to_globals_misc is declared as const pointer

[Ralf Friedl]

Mike Frysinger wrote:

On 20 Jan 2018 19:03, Ralf Friedl wrote:

Mike Frysinger wrote:

the pointer itself is the thing that is const, not the memory it points to.
this lets the compiler optimize the loads by generating relocations via the
pointer ... there's the fixup at the initial load time, but after that, it's
just offsets to a constant memory location.  but removing the const markings,
the compiler has to reload the pointer everytime.

Actually, why does the comipler have to reload the pointer? In your
example, there is no other function called between accesses to the
pointer that might change the pointer, and anti-aliasing should let the
compiler know that the memory modified through the pointer can't be the
pointer itself because of the type difference.

when i say reload, i don't mean in a single function.  i agree as you say that
the compiler has no need based on its memory model.  i meant that on every call,
the pointer has to be loaded because the compiler cannot know that it hasn't
changed.  by having the pointer be const, gcc knows that it can be referred to
directly.

it's a pretty esoteric optimization/hack, but busybox has a lot of these things,
and when you add them up together, it makes for a difference.  i'm not sure how
this can be written portably though since we're pushing the limits of the
compiler :/.
You say you don't mean in a single function, but your example with 
raise_exception shows that it does happen within a single function, so 
the question is, why does the compiler not recognize it?


Maybe because anti-aliasing is not enabled? I tried to compile busybox 
with -O3 instead of -O2, and I get at least some warnings, and together 
with -Wall they abort the compilation. I had to add 
"-Wno-error=format-overflow -Wno-error=format-truncation" to 
EXTRA_CFLAGS for gcc 7.2.1. One could argue whether gcc should warn for 
snprintf (in contrast to sprintf), but at the moment it does, at least 
unless the return value is used.

___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: ASH: why ash_ptr_to_globals_misc is declared as const pointer

[Ralf Friedl]

Mike Frysinger schrieb:

On 22 Jan 2018 09:23, Yunlian Jiang wrote:

On Fri, Jan 19, 2018 at 3:32 PM, Mike Frysinger wrote:

is it that busybox is crashing ?  is clang/lld placing this pointer in
const
memory (even though we have section(".data")) ?  or is it generating an
abort
when it reaches what it thinks is an undefined situation (like trying to
write
to a const memory location in INIT_G_var()) ?


The busybox is crashing. On arm boards, the command line
'busybox ash' gave me a segmentation fault.

In the ash_ptr_hack.c, it sets the attribute  __attribute__ (section
(".data")) when
'GCC_COMBINE' is defined.  But clang does not have this MACRO defined.

i don't think GCC_COMBINE is relevant, so just forget about it.  ash_ptr_hack.c
does:
   struct globals_misc *ash_ptr_to_globals_misc;
that means that pointer should end up the .data/writable memory by default.

the fact that another compilation unit declares it as const isn't terribly
relevant ... the actual storage should still be writable.  you should check
that is happening (or if it's ending up somewhere else).  we've seen llvm
incorrectly propagate attributes to storage before based on decls seen in
other modules.
GCC_COMBINE is relevant because it changes the definitions aof the 
pointers. without GCC_COMBINE, they are defined as normal variables, and 
every compiler should be able to compile them. With GCC_COMBINE they are 
defined as const but with section ".data", and a compiler that doesn't 
understand that will fail. in busybox GCC_COMBINE is only set in 
scripts/Makefile.IMA, together with -combine and -fwhole-program. The 
first line of this script says "This is completely unsupported." A 
compiler that doesn't support -combine and -fwhole-program doesn't need 
to define GCC_COMBINE and won't have a problem with the code. If the 
compiler supports the concept of -fwhole-program, the trick to use a 
different definition and declaration in different files no longer works. 
But maybe then the compiler is smart enough to realize that there are no 
other assignments to the pointers so it can omit the reload. If not, 
something like the ".data" section is necessary, and if the compiler has 
-fwhole-program but no ".data", then this can't be used.



___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: ASH: why ash_ptr_to_globals_misc is declared as const pointer

[Mike Frysinger]

On 22 Jan 2018 09:23, Yunlian Jiang wrote:
> On Fri, Jan 19, 2018 at 3:32 PM, Mike Frysinger wrote:
> > On 18 Jan 2018 13:48, Yunlian Jiang wrote:
> > >I tried to build busybox with clang and use it to create recovery
> > image
> > > for ChromeOS.
> > > It fails to recover an arm based ChromeBook.
> >
> > is it that busybox is crashing ?  is clang/lld placing this pointer in
> > const
> > memory (even though we have section(".data")) ?  or is it generating an
> > abort
> > when it reaches what it thinks is an undefined situation (like trying to
> > write
> > to a const memory location in INIT_G_var()) ?
> >
> The busybox is crashing. On arm boards, the command line
> 'busybox ash' gave me a segmentation fault.
> 
> In the ash_ptr_hack.c, it sets the attribute  __attribute__ (section
> (".data")) when
> 'GCC_COMBINE' is defined.  But clang does not have this MACRO defined.

i don't think GCC_COMBINE is relevant, so just forget about it.  ash_ptr_hack.c
does:
  struct globals_misc *ash_ptr_to_globals_misc;
that means that pointer should end up the .data/writable memory by default.

the fact that another compilation unit declares it as const isn't terribly
relevant ... the actual storage should still be writable.  you should check
that is happening (or if it's ending up somewhere else).  we've seen llvm
incorrectly propagate attributes to storage before based on decls seen in
other modules.

> > >I digged a little bit.
> > >Below patch makes it work.
> >
> > your patch only changed two places ... presumably if this global ptr
> > trickery
> > isn't working here, it doesn't work in any of them.
> >
> My patch changed the the declarations in the ash.c, did I miss something?

if you look in ash_ptr_hack.c, we have three pointers doing this kind of
trickery, but you only changed two.  if you grep the tree for GCC_COMBINE
or const.*attribute.*section.*data, you see we use the same trick in other
places in busybox.  so if it's not working in ash, i don't expect it to
work for any of these.

> > > My question is.
> > >the  ash_ptr_to_globals_misc, ash_ptr_to_globals_memstack
> > > and ash_ptr_to_globals_var
> > > are defined in ash_ptr_hack.c as normal pointers. But in ash.c, they are
> > > declared as const
> > > pointers. What is the benefit of doing that?
> >
> > the pointer itself is the thing that is const, not the memory it points to.
> > this lets the compiler optimize the loads by generating relocations via the
> > pointer ... there's the fixup at the initial load time, but after that,
> > it's
> > just offsets to a constant memory location.  but removing the const
> > markings,
> > the compiler has to reload the pointer everytime.
>
> Can this trick be included only in configurations for smaller code size?

before we try doing something like that, we really need to understand why it's
failing in the first place.  we don't want to just shove things under the carpet
and hope no one notices the bumps.
-mike


signature.asc
Description: Digital signature
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: ASH: why ash_ptr_to_globals_misc is declared as const pointer

[Mike Frysinger]

On 20 Jan 2018 19:03, Ralf Friedl wrote:
> Mike Frysinger wrote:
> > the pointer itself is the thing that is const, not the memory it points to.
> > this lets the compiler optimize the loads by generating relocations via the
> > pointer ... there's the fixup at the initial load time, but after that, it's
> > just offsets to a constant memory location.  but removing the const 
> > markings,
> > the compiler has to reload the pointer everytime.
> 
> Actually, why does the comipler have to reload the pointer? In your 
> example, there is no other function called between accesses to the 
> pointer that might change the pointer, and anti-aliasing should let the 
> compiler know that the memory modified through the pointer can't be the 
> pointer itself because of the type difference.

when i say reload, i don't mean in a single function.  i agree as you say that
the compiler has no need based on its memory model.  i meant that on every call,
the pointer has to be loaded because the compiler cannot know that it hasn't
changed.  by having the pointer be const, gcc knows that it can be referred to
directly.

it's a pretty esoteric optimization/hack, but busybox has a lot of these things,
and when you add them up together, it makes for a difference.  i'm not sure how
this can be written portably though since we're pushing the limits of the 
compiler :/.
-mike


signature.asc
Description: Digital signature
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: ASH: why ash_ptr_to_globals_misc is declared as const pointer

[Ralf Friedl]

Mike Frysinger wrote:

the pointer itself is the thing that is const, not the memory it points to.
this lets the compiler optimize the loads by generating relocations via the
pointer ... there's the fixup at the initial load time, but after that, it's
just offsets to a constant memory location.  but removing the const markings,
the compiler has to reload the pointer everytime.
Actually, why does the comipler have to reload the pointer? In your 
example, there is no other function called between accesses to the 
pointer that might change the pointer, and anti-aliasing should let the 
compiler know that the memory modified through the pointer can't be the 
pointer itself because of the type difference.

you can check this yourself.  this is the delta from removing the const:
   546041222 488   56314dbfa busybox.const
   549641222 488   56674dd62 busybox.no.const
+360+360

and an example from the disassembly.

const/ :
0:  48 83 ec 08 sub$0x8,%rsp
4:  48 8b 05 00 00 00 00mov0x0(%rip),%rax# b 

7: R_X86_64_PC32ash_ptr_to_globals_misc-0x4
b:  8b 50 38mov0x38(%rax),%edx
e:  ff c2   inc%edx
   10:  89 50 38mov%edx,0x38(%rax)
   13:  40 88 78 3f mov%dil,0x3f(%rax)
   17:  48 8b 78 30 mov0x30(%rax),%rdi
   1b:  be 01 00 00 00  mov$0x1,%esi
   20:  e8 00 00 00 00  callq  25 
21: R_X86_64_PC32   __longjmp_chk-0x4

no.const/ :
0:  48 83 ec 08 sub$0x8,%rsp
4:  48 8b 15 00 00 00 00mov0x0(%rip),%rdx# b 

7: R_X86_64_PC32ash_ptr_to_globals_misc-0x4
b:  8b 42 38mov0x38(%rdx),%eax
e:  ff c0   inc%eax
   10:  89 42 38mov%eax,0x38(%rdx)

extra load <<<

  13:   48 8b 05 00 00 00 00mov0x0(%rip),%rax# 1a 

16: R_X86_64_PC32   ash_ptr_to_globals_misc-0x4
  1a:   be 01 00 00 00  mov$0x1,%esi

   1f:  40 88 78 3f mov%dil,0x3f(%rax)
   23:  48 8b 78 30 mov0x30(%rax),%rdi
   27:  e8 00 00 00 00  callq  2c 
28: R_X86_64_PC32   __longjmp_chk-0x4

would be nice if we documented all the little tricks we used somewhere.

___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: ASH: why ash_ptr_to_globals_misc is declared as const pointer

[Mike Frysinger]

On 20 Jan 2018 12:18, Xabier Oneca -- xOneca wrote:
> >> My question is.
> >>the  ash_ptr_to_globals_misc, ash_ptr_to_globals_memstack
> >> and ash_ptr_to_globals_var
> >> are defined in ash_ptr_hack.c as normal pointers. But in ash.c, they are
> >> declared as const
> >> pointers. What is the benefit of doing that?
> >
> > the pointer itself is the thing that is const, not the memory it points to.
> > this lets the compiler optimize the loads by generating relocations via the
> > pointer ... there's the fixup at the initial load time, but after that, it's
> > just offsets to a constant memory location.  but removing the const 
> > markings,
> > the compiler has to reload the pointer everytime.
> 
> Then why is "cheating" in the header file defining it non-const?

there is no header file.  ash_ptr_hack.c is the backing storage which is in a
separate compilation unit from ash.c.  the decl is in ash.c sep from the 
storage.
-mike


signature.asc
Description: Digital signature
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: ASH: why ash_ptr_to_globals_misc is declared as const pointer

[Xabier Oneca -- xOneca]

Hi Mike,

>> My question is.
>>the  ash_ptr_to_globals_misc, ash_ptr_to_globals_memstack
>> and ash_ptr_to_globals_var
>> are defined in ash_ptr_hack.c as normal pointers. But in ash.c, they are
>> declared as const
>> pointers. What is the benefit of doing that?
>
> the pointer itself is the thing that is const, not the memory it points to.
> this lets the compiler optimize the loads by generating relocations via the
> pointer ... there's the fixup at the initial load time, but after that, it's
> just offsets to a constant memory location.  but removing the const markings,
> the compiler has to reload the pointer everytime.

Then why is "cheating" in the header file defining it non-const?

Cheers,
Xabier Oneca_,,_
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: ASH: why ash_ptr_to_globals_misc is declared as const pointer

[Mike Frysinger]

On 18 Jan 2018 13:48, Yunlian Jiang wrote:
>I tried to build busybox with clang and use it to create recovery image
> for ChromeOS.
> It fails to recover an arm based ChromeBook.

is it that busybox is crashing ?  is clang/lld placing this pointer in const
memory (even though we have section(".data")) ?  or is it generating an abort
when it reaches what it thinks is an undefined situation (like trying to write
to a const memory location in INIT_G_var()) ?

>I digged a little bit.
>Below patch makes it work.

your patch only changed two places ... presumably if this global ptr trickery
isn't working here, it doesn't work in any of them.

> My question is.
>the  ash_ptr_to_globals_misc, ash_ptr_to_globals_memstack
> and ash_ptr_to_globals_var
> are defined in ash_ptr_hack.c as normal pointers. But in ash.c, they are
> declared as const
> pointers. What is the benefit of doing that?

the pointer itself is the thing that is const, not the memory it points to.
this lets the compiler optimize the loads by generating relocations via the
pointer ... there's the fixup at the initial load time, but after that, it's
just offsets to a constant memory location.  but removing the const markings,
the compiler has to reload the pointer everytime.

you can check this yourself.  this is the delta from removing the const:
  546041222 488   56314dbfa busybox.const
  549641222 488   56674dd62 busybox.no.const
   +360+360

and an example from the disassembly.

const/ :
   0:   48 83 ec 08 sub$0x8,%rsp
   4:   48 8b 05 00 00 00 00mov0x0(%rip),%rax# b 

7: R_X86_64_PC32ash_ptr_to_globals_misc-0x4
   b:   8b 50 38mov0x38(%rax),%edx
   e:   ff c2   inc%edx
  10:   89 50 38mov%edx,0x38(%rax)
  13:   40 88 78 3f mov%dil,0x3f(%rax)
  17:   48 8b 78 30 mov0x30(%rax),%rdi
  1b:   be 01 00 00 00  mov$0x1,%esi
  20:   e8 00 00 00 00  callq  25 
21: R_X86_64_PC32   __longjmp_chk-0x4

no.const/ :
   0:   48 83 ec 08 sub$0x8,%rsp
   4:   48 8b 15 00 00 00 00mov0x0(%rip),%rdx# b 

7: R_X86_64_PC32ash_ptr_to_globals_misc-0x4
   b:   8b 42 38mov0x38(%rdx),%eax
   e:   ff c0   inc%eax
  10:   89 42 38mov%eax,0x38(%rdx)
>>> extra load <<<
>  13:  48 8b 05 00 00 00 00mov0x0(%rip),%rax# 1a 
> 
>   16: R_X86_64_PC32   ash_ptr_to_globals_misc-0x4
>  1a:  be 01 00 00 00  mov$0x1,%esi
  1f:   40 88 78 3f mov%dil,0x3f(%rax)
  23:   48 8b 78 30 mov0x30(%rax),%rdi
  27:   e8 00 00 00 00  callq  2c 
28: R_X86_64_PC32   __longjmp_chk-0x4

would be nice if we documented all the little tricks we used somewhere.
-mike


signature.asc
Description: Digital signature
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: ASH: why ash_ptr_to_globals_misc is declared as const pointer

[Xabier Oneca -- xOneca]

Hi,

> There also checks for GCC_COMBINE, but it does the opposite of your patch.

Sorry, my bad. It does the same! :/

Cheers,
Xabier Oneca_,,_

--
Que te lo pases bien,
xOneca_,,_


2018-01-18 23:02 GMT+01:00 Xabier Oneca  --  xOneca :
> Hello,
>
> 2018-01-18 22:48 GMT+01:00 Yunlian Jiang :
>> My question is.
>>the  ash_ptr_to_globals_misc, ash_ptr_to_globals_memstack and
>> ash_ptr_to_globals_var
>> are defined in ash_ptr_hack.c as normal pointers. But in ash.c, they are
>> declared as const
>> pointers. What is the benefit of doing that?
>
> Maybe same purpose as ptr_to_globals?
>
> https://git.busybox.net/busybox/tree/libbb/ptr_to_globals.c#n13
>
> There also checks for GCC_COMBINE, but it does the opposite of your patch.
>
> Cheers,
> Xabier Oneca_,,_
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: ASH: why ash_ptr_to_globals_misc is declared as const pointer

[Xabier Oneca -- xOneca]

Hello,

2018-01-18 22:48 GMT+01:00 Yunlian Jiang :
> My question is.
>the  ash_ptr_to_globals_misc, ash_ptr_to_globals_memstack and
> ash_ptr_to_globals_var
> are defined in ash_ptr_hack.c as normal pointers. But in ash.c, they are
> declared as const
> pointers. What is the benefit of doing that?

Maybe same purpose as ptr_to_globals?

https://git.busybox.net/busybox/tree/libbb/ptr_to_globals.c#n13

There also checks for GCC_COMBINE, but it does the opposite of your patch.

Cheers,
Xabier Oneca_,,_
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox