diff mbox series

[v1,03/11] tools/nolibc: include crt.h before arch.h

Message ID c61b5bc53895e8c6b2f30d59f86067973e6bbce0.1687976753.git.falcon@tinylab.org (mailing list archive)
State Handled Elsewhere
Headers show
Series tools/nolibc: shrink arch support | expand

Checks

Context Check Description
conchuod/tree_selection fail Failed to apply to next/pending-fixes, riscv/for-next or riscv/master

Commit Message

Zhangjin Wu June 28, 2023, 6:54 p.m. UTC
The crt.h provides a new _start_c() function, which is required by the
new assembly _start entry of arch-<ARCH>.h (included by arch.h), let's
include crt.h before arch.h.

This '#include "crt.h"' doesn't let the new _start_c() work immediately,
but it is a base of the coming patches to move most of the assembly
_start operations to the _start_c() function for every supported
architecture.

Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
---
 tools/include/nolibc/Makefile | 1 +
 tools/include/nolibc/nolibc.h | 1 +
 tools/include/nolibc/signal.h | 1 +
 tools/include/nolibc/stdio.h  | 1 +
 tools/include/nolibc/stdlib.h | 1 +
 tools/include/nolibc/sys.h    | 1 +
 tools/include/nolibc/time.h   | 1 +
 tools/include/nolibc/unistd.h | 1 +
 8 files changed, 8 insertions(+)

Comments

Thomas Weißschuh July 2, 2023, 6:57 p.m. UTC | #1
On 2023-06-29 02:54:35+0800, Zhangjin Wu wrote:
> The crt.h provides a new _start_c() function, which is required by the
> new assembly _start entry of arch-<ARCH>.h (included by arch.h), let's
> include crt.h before arch.h.
> 
> This '#include "crt.h"' doesn't let the new _start_c() work immediately,
> but it is a base of the coming patches to move most of the assembly
> _start operations to the _start_c() function for every supported
> architecture.

Why don't the arch-*.h files include this new header?
They are the users of the new symbol.

> 
> Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
> ---
>  tools/include/nolibc/Makefile | 1 +
>  tools/include/nolibc/nolibc.h | 1 +
>  tools/include/nolibc/signal.h | 1 +
>  tools/include/nolibc/stdio.h  | 1 +
>  tools/include/nolibc/stdlib.h | 1 +
>  tools/include/nolibc/sys.h    | 1 +
>  tools/include/nolibc/time.h   | 1 +
>  tools/include/nolibc/unistd.h | 1 +
>  8 files changed, 8 insertions(+)
> 
> diff --git a/tools/include/nolibc/Makefile b/tools/include/nolibc/Makefile
> index 875e13e3c851..00471e59b11e 100644
> --- a/tools/include/nolibc/Makefile
> +++ b/tools/include/nolibc/Makefile
> @@ -37,6 +37,7 @@ NARCH            = $(or $(NARCH_$(ARCH)),$(ARCH))
>  arch_file := arch-$(NARCH).h
>  all_files := \
>  		compiler.h \
> +		crt.h \

This should be part of the patch adding the file.

>  		ctype.h \
>  		errno.h \
>  		nolibc.h \
> diff --git a/tools/include/nolibc/nolibc.h b/tools/include/nolibc/nolibc.h
> index 1f8d821000ac..2cc9ccd90d56 100644
> --- a/tools/include/nolibc/nolibc.h
> +++ b/tools/include/nolibc/nolibc.h
> @@ -93,6 +93,7 @@
>  #define _NOLIBC_H
>  
>  #include "std.h"
> +#include "crt.h"
>  #include "arch.h"
>  #include "types.h"
>  #include "sys.h"
> diff --git a/tools/include/nolibc/signal.h b/tools/include/nolibc/signal.h
> index 137552216e46..f0a1418c1cb2 100644
> --- a/tools/include/nolibc/signal.h
> +++ b/tools/include/nolibc/signal.h
> @@ -8,6 +8,7 @@
>  #define _NOLIBC_SIGNAL_H
>  
>  #include "std.h"
> +#include "crt.h"
>  #include "arch.h"
>  #include "types.h"
>  #include "sys.h"
> diff --git a/tools/include/nolibc/stdio.h b/tools/include/nolibc/stdio.h
> index 0eef91daf289..89d3749b3620 100644
> --- a/tools/include/nolibc/stdio.h
> +++ b/tools/include/nolibc/stdio.h
> @@ -10,6 +10,7 @@
>  #include <stdarg.h>
>  
>  #include "std.h"
> +#include "crt.h"
>  #include "arch.h"
>  #include "errno.h"
>  #include "types.h"
> diff --git a/tools/include/nolibc/stdlib.h b/tools/include/nolibc/stdlib.h
> index 902162f80337..0ff7fac40bd4 100644
> --- a/tools/include/nolibc/stdlib.h
> +++ b/tools/include/nolibc/stdlib.h
> @@ -8,6 +8,7 @@
>  #define _NOLIBC_STDLIB_H
>  
>  #include "std.h"
> +#include "crt.h"
>  #include "arch.h"
>  #include "types.h"
>  #include "sys.h"
> diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h
> index 2c302f3feb71..b6c33c40c037 100644
> --- a/tools/include/nolibc/sys.h
> +++ b/tools/include/nolibc/sys.h
> @@ -24,6 +24,7 @@
>  #include <linux/reboot.h> /* for LINUX_REBOOT_* */
>  #include <linux/prctl.h>
>  
> +#include "crt.h"
>  #include "arch.h"
>  #include "errno.h"
>  #include "types.h"
> diff --git a/tools/include/nolibc/time.h b/tools/include/nolibc/time.h
> index 84655361b9ad..bbe8f9aa3e9b 100644
> --- a/tools/include/nolibc/time.h
> +++ b/tools/include/nolibc/time.h
> @@ -8,6 +8,7 @@
>  #define _NOLIBC_TIME_H
>  
>  #include "std.h"
> +#include "crt.h"
>  #include "arch.h"
>  #include "types.h"
>  #include "sys.h"
> diff --git a/tools/include/nolibc/unistd.h b/tools/include/nolibc/unistd.h
> index e38f3660c051..f1677224bb5a 100644
> --- a/tools/include/nolibc/unistd.h
> +++ b/tools/include/nolibc/unistd.h
> @@ -8,6 +8,7 @@
>  #define _NOLIBC_UNISTD_H
>  
>  #include "std.h"
> +#include "crt.h"
>  #include "arch.h"
>  #include "types.h"
>  #include "sys.h"
> -- 
> 2.25.1
>
Zhangjin Wu July 3, 2023, 9:58 a.m. UTC | #2
Hi, Thomas

> 
> On 2023-06-29 02:54:35+0800, Zhangjin Wu wrote:
> > The crt.h provides a new _start_c() function, which is required by the
> > new assembly _start entry of arch-<ARCH>.h (included by arch.h), let's
> > include crt.h before arch.h.
> > 
> > This '#include "crt.h"' doesn't let the new _start_c() work immediately,
> > but it is a base of the coming patches to move most of the assembly
> > _start operations to the _start_c() function for every supported
> > architecture.
> 
> Why don't the arch-*.h files include this new header?
> They are the users of the new symbol.
>

I have tried so, but since crt.h itself is not architecture specific, add it
before arch.h will avoid every new arch porting add the same thing again and
again, currently, we only need to add once. I have even planned to move
compiler.h out of arch-*.h, but not yet ;-)

And also, crt.h may require more features in the future, like init/fini
support, it may be not only used by arch-*.h files.

> > 
> > Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
> > ---
> >  tools/include/nolibc/Makefile | 1 +
> >  tools/include/nolibc/nolibc.h | 1 +
> >  tools/include/nolibc/signal.h | 1 +
> >  tools/include/nolibc/stdio.h  | 1 +
> >  tools/include/nolibc/stdlib.h | 1 +
> >  tools/include/nolibc/sys.h    | 1 +
> >  tools/include/nolibc/time.h   | 1 +
> >  tools/include/nolibc/unistd.h | 1 +
> >  8 files changed, 8 insertions(+)
> > 
> > diff --git a/tools/include/nolibc/Makefile b/tools/include/nolibc/Makefile
> > index 875e13e3c851..00471e59b11e 100644
> > --- a/tools/include/nolibc/Makefile
> > +++ b/tools/include/nolibc/Makefile
> > @@ -37,6 +37,7 @@ NARCH            = $(or $(NARCH_$(ARCH)),$(ARCH))
> >  arch_file := arch-$(NARCH).h
> >  all_files := \
> >  		compiler.h \
> > +		crt.h \
> 
> This should be part of the patch adding the file.
>

Ok, thanks very much.

Best regards,
Zhangjin

> >  		ctype.h \
> >  		errno.h \
> >  		nolibc.h \
Thomas Weißschuh July 3, 2023, 10:11 a.m. UTC | #3
On 2023-07-03 17:58:32+0800, Zhangjin Wu wrote:
> Hi, Thomas
> 
> > 
> > On 2023-06-29 02:54:35+0800, Zhangjin Wu wrote:
> > > The crt.h provides a new _start_c() function, which is required by the
> > > new assembly _start entry of arch-<ARCH>.h (included by arch.h), let's
> > > include crt.h before arch.h.
> > > 
> > > This '#include "crt.h"' doesn't let the new _start_c() work immediately,
> > > but it is a base of the coming patches to move most of the assembly
> > > _start operations to the _start_c() function for every supported
> > > architecture.
> > 
> > Why don't the arch-*.h files include this new header?
> > They are the users of the new symbol.
> >
> 
> I have tried so, but since crt.h itself is not architecture specific, add it
> before arch.h will avoid every new arch porting add the same thing again and
> again, currently, we only need to add once. I have even planned to move
> compiler.h out of arch-*.h, but not yet ;-)

While this saves a few lines of code in my opinion it hurts clarity to
rely on implicitly pre-included things.

> every new arch porting

That doesn't seem like a very frequent occurrence :-)

> And also, crt.h may require more features in the future, like init/fini
> support, it may be not only used by arch-*.h files.

Do you have a mechanism in mind that supports init/fini without needing
an ELF parser at runtime? I guess an ELF parser would make it a complete
no-go.

Also the value added by init/fini seems fairly limited for a statically
linked (tiny) application.

> [..]
Zhangjin Wu July 3, 2023, 2:55 p.m. UTC | #4
> On 2023-07-03 17:58:32+0800, Zhangjin Wu wrote:
> > Hi, Thomas
> > 
> > > 
> > > On 2023-06-29 02:54:35+0800, Zhangjin Wu wrote:
> > > > The crt.h provides a new _start_c() function, which is required by the
> > > > new assembly _start entry of arch-<ARCH>.h (included by arch.h), let's
> > > > include crt.h before arch.h.
> > > > 
> > > > This '#include "crt.h"' doesn't let the new _start_c() work immediately,
> > > > but it is a base of the coming patches to move most of the assembly
> > > > _start operations to the _start_c() function for every supported
> > > > architecture.
> > > 
> > > Why don't the arch-*.h files include this new header?
> > > They are the users of the new symbol.
> > >
> > 
> > I have tried so, but since crt.h itself is not architecture specific, add it
> > before arch.h will avoid every new arch porting add the same thing again and
> > again, currently, we only need to add once. I have even planned to move
> > compiler.h out of arch-*.h, but not yet ;-)
> 
> While this saves a few lines of code in my opinion it hurts clarity to
> rely on implicitly pre-included things.
>

To be clearer, what about split the arch.h to sys_arch.h (my_syscall*)
and crt_arch.h? (_start part) and then, we can include crt_arch.h in
crt.h and at the same time, include sys_arch.h in sys.h, and at last
we need to create a <ARCH> directory for the them.

    crt.h:
        #include "crt_arch.h"

	_start_c ()

    sys.h:
        #include "sys_arch.h"

	sys_xxx()
	{
            my_syscall<N>(...)
	}

    crt_arch.h:

        #ifdef ARCH
        #include "<ARCH>/crt_arch.h"
	#endif

    sys_arch.h:

        #ifdef ARCH
        #inculde "<ARCH>/sys_arch.h"
	#endif

I just found musl uses such structure ;-)

> > every new arch porting
> 
> That doesn't seem like a very frequent occurrence :-)
>

Yes, it is not often.

> > And also, crt.h may require more features in the future, like init/fini
> > support, it may be not only used by arch-*.h files.
> 
> Do you have a mechanism in mind that supports init/fini without needing
> an ELF parser at runtime? I guess an ELF parser would make it a complete
> no-go.
>

I didn't really think about this yet ;-)

> Also the value added by init/fini seems fairly limited for a statically
> linked (tiny) application.
>

Yeah.

Thanks,
Zhangjin

> > [..]
diff mbox series

Patch

diff --git a/tools/include/nolibc/Makefile b/tools/include/nolibc/Makefile
index 875e13e3c851..00471e59b11e 100644
--- a/tools/include/nolibc/Makefile
+++ b/tools/include/nolibc/Makefile
@@ -37,6 +37,7 @@  NARCH            = $(or $(NARCH_$(ARCH)),$(ARCH))
 arch_file := arch-$(NARCH).h
 all_files := \
 		compiler.h \
+		crt.h \
 		ctype.h \
 		errno.h \
 		nolibc.h \
diff --git a/tools/include/nolibc/nolibc.h b/tools/include/nolibc/nolibc.h
index 1f8d821000ac..2cc9ccd90d56 100644
--- a/tools/include/nolibc/nolibc.h
+++ b/tools/include/nolibc/nolibc.h
@@ -93,6 +93,7 @@ 
 #define _NOLIBC_H
 
 #include "std.h"
+#include "crt.h"
 #include "arch.h"
 #include "types.h"
 #include "sys.h"
diff --git a/tools/include/nolibc/signal.h b/tools/include/nolibc/signal.h
index 137552216e46..f0a1418c1cb2 100644
--- a/tools/include/nolibc/signal.h
+++ b/tools/include/nolibc/signal.h
@@ -8,6 +8,7 @@ 
 #define _NOLIBC_SIGNAL_H
 
 #include "std.h"
+#include "crt.h"
 #include "arch.h"
 #include "types.h"
 #include "sys.h"
diff --git a/tools/include/nolibc/stdio.h b/tools/include/nolibc/stdio.h
index 0eef91daf289..89d3749b3620 100644
--- a/tools/include/nolibc/stdio.h
+++ b/tools/include/nolibc/stdio.h
@@ -10,6 +10,7 @@ 
 #include <stdarg.h>
 
 #include "std.h"
+#include "crt.h"
 #include "arch.h"
 #include "errno.h"
 #include "types.h"
diff --git a/tools/include/nolibc/stdlib.h b/tools/include/nolibc/stdlib.h
index 902162f80337..0ff7fac40bd4 100644
--- a/tools/include/nolibc/stdlib.h
+++ b/tools/include/nolibc/stdlib.h
@@ -8,6 +8,7 @@ 
 #define _NOLIBC_STDLIB_H
 
 #include "std.h"
+#include "crt.h"
 #include "arch.h"
 #include "types.h"
 #include "sys.h"
diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h
index 2c302f3feb71..b6c33c40c037 100644
--- a/tools/include/nolibc/sys.h
+++ b/tools/include/nolibc/sys.h
@@ -24,6 +24,7 @@ 
 #include <linux/reboot.h> /* for LINUX_REBOOT_* */
 #include <linux/prctl.h>
 
+#include "crt.h"
 #include "arch.h"
 #include "errno.h"
 #include "types.h"
diff --git a/tools/include/nolibc/time.h b/tools/include/nolibc/time.h
index 84655361b9ad..bbe8f9aa3e9b 100644
--- a/tools/include/nolibc/time.h
+++ b/tools/include/nolibc/time.h
@@ -8,6 +8,7 @@ 
 #define _NOLIBC_TIME_H
 
 #include "std.h"
+#include "crt.h"
 #include "arch.h"
 #include "types.h"
 #include "sys.h"
diff --git a/tools/include/nolibc/unistd.h b/tools/include/nolibc/unistd.h
index e38f3660c051..f1677224bb5a 100644
--- a/tools/include/nolibc/unistd.h
+++ b/tools/include/nolibc/unistd.h
@@ -8,6 +8,7 @@ 
 #define _NOLIBC_UNISTD_H
 
 #include "std.h"
+#include "crt.h"
 #include "arch.h"
 #include "types.h"
 #include "sys.h"