Fix conflicts to support Newlib4.4 and pthread-embedded #67
Conversation
| LIBLWIP_CFLAGS-$(call have_clang) += -Wno-macro-redefined | ||
| LIBLWIP_CFLAGS-$(CONFIG_LWIP_DEBUG) += -DUK_DEBUG | ||
| LIBLWIP_CFLAGS-y += -D__IN_LIBLWIP__ | ||
| LIBLWIP_CFLAGS-y += -DWORKAROUND_NEWLIB |
There was a problem hiding this comment.
Maybe this could have a better name?
There was a problem hiding this comment.
This name is suggested by chatGPT :D , if you think it is a bad suggestion I can change it ;)
There was a problem hiding this comment.
Is this actually needed here? I don't see any changes related to this.
| #if ((defined CONFIG_ARCH_X86_64) || (defined CONFIG_ARCH_ARM_64)) | ||
| #include <endian.h> | ||
| #endif |
There was a problem hiding this comment.
Why is this for x86 and arm only? I know we don't support the others that well, but do you believe that this would not work with RISCV for example?
There was a problem hiding this comment.
This code works for any 64-bit little-endian platform. I added this macro because some libraries, like lwIP, depend on the endianness of the system. When we add support for a new architecture, we must also add it here. If we don’t, we will get an error. That error is on purpose, it shows us that this code depends on the platform, and we need to update it (if it is needed for new platform). In this way, the errors help us remember what to consider when porting to new platforms.
There was a problem hiding this comment.
I think it's fine to not error on new architectures. It's expected for lwip to depend on endianess, and the point of endian.h is to make it transparent. So, I would remove the ifdefs.
StefanJum
left a comment
StefanJum
left a comment
There was a problem hiding this comment.
Looks good, see the few comments, and also follow the general instructions from here unikraft/lib-python3#24
| LIBLWIP_CFLAGS-$(call have_clang) += -Wno-macro-redefined | ||
| LIBLWIP_CFLAGS-$(CONFIG_LWIP_DEBUG) += -DUK_DEBUG | ||
| LIBLWIP_CFLAGS-y += -D__IN_LIBLWIP__ | ||
| LIBLWIP_CFLAGS-y += -DWORKAROUND_NEWLIB |
There was a problem hiding this comment.
Is this actually needed here? I don't see any changes related to this.
| #if ((defined CONFIG_ARCH_X86_64) || (defined CONFIG_ARCH_ARM_64)) | ||
| #include <endian.h> | ||
| #endif |
There was a problem hiding this comment.
I think it's fine to not error on new architectures. It's expected for lwip to depend on endianess, and the point of endian.h is to make it transparent. So, I would remove the ifdefs.
Makefile.uk
Outdated
| LIBLWIP_SRCS-$(CONFIG_LWIP_THREADS) += $(LIBLWIP_BASE)/threads.c|unikraft | ||
| LIBLWIP_SRCS-y += $(LIBLWIP_BASE)/init.c|unikraft | ||
| LIBLWIP_SRCS-y += $(LIBLWIP_BASE)/time.c|unikraft | ||
| LIBLWIP_SRCS-y += $(LIBLWIP_BASE)/sendfile.c|unikraft |
There was a problem hiding this comment.
Why is this not needed anymore? Is it the same for musl builds?
3 participants
No description provided.