Skip to content

Conversation

@luckyluke
Copy link

@luckyluke luckyluke commented Dec 11, 2018

Basic support for i386, should fix #341


This change is Reviewable

Signed-off-by: Luca Dariz <luca.dariz@gmail.com>
lkl_get_free_irq lkl_put_irq lkl_is_running lkl_bug lkl_printf

ifeq ($(OUTPUT_FORMAT),elf32-i386)
LKL_ENTRY_POINTS += \
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The symbols added here are not "LKL_ENTRY_POINTS", right? I have no idea what to call the variable used in the objcopy-invocation should be called like insteadm though, So maybe we have one variable that we fill conditionally (if i386) add these thunk-symbols and then unconditionally add LKL_ENTRY_POINTS. As I said, though I have a hard time coming up with a name.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, i'll update the patch in the next days

@aostrouhhov
Copy link

aostrouhhov commented Dec 14, 2018

It should be mentioned that, I suppose, different compiler versions are looking for different symbol names: sometimes it is __x86.get_pc_thunk.REG, sometimes __i386.get_pc_thunk.REG or __i686.get_pc_thunk.REG. I don't know how many of them there are.

@luckyluke
Copy link
Author

luckyluke commented Dec 16, 2018

It should be mentioned that, I suppose, different compiler versions are looking for different symbol names

Thank you, I didn't think about that. Any idea how to detect the compiler? From a quick grep I didn't find examples in linux, maybe something like
ifneq ($(shell $(CC) --version | grep gcc),)
to select which compiler-specific symbols to keep? I don't know which compilers can be used except gcc.

@luckyluke luckyluke force-pushed the lkl_i386_fix branch 2 times, most recently from 8bd2734 to c520f30 Compare December 16, 2018 15:25
@luckyluke
Copy link
Author

I tried with 32-bit clang and no modifications are necessary, everything compiles and test pass as well. Unfortunately I can't test other compilers like intel icc.

Copy link
Member

@thehajime thehajime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think keep using LKL_ENTRY_POINTS would be nicer.

@luckyluke
Copy link
Author

removed the commit with the LKL_ENTRY_POINTS modification

@thehajime
Copy link
Member

thanks @luckyluke. the update is fine.

another thought is if we can have CI (circleci) for i386 build/tests. it would be great if you added an i386 build instance for circleci if possible ?

current our CI is running here:

https://circleci.com/gh/lkl/workflows/linux/tree/master

@luckyluke
Copy link
Author

Hi @thehajime, thank you for the feedback.
I'll try to add the circleci conf in the next days.

luckyluke and others added 3 commits March 22, 2019 14:43
Signed-off-by: Luca Dariz <luca.dariz@gmail.com>
Signed-off-by: Luca Dariz <luca.dariz@gmail.com>
Signed-off-by: Luca Dariz <luca.dariz@gmail.com>
@luckyluke
Copy link
Author

Hi @thehajime I added a circleci config on a container I created based on ubuntu i386. This required some more fix for the tests, I guess due to different versions of gcc and btrfs-tools from my debian machine.

@thehajime
Copy link
Member

Hi @thehajime I added a circleci config on a container I created based on ubuntu i386.

Thanks ! LGTM.

This required some more fix for the tests, I guess due to different versions of gcc and btrfs-tools from my debian machine.

for the btrfs-tools, it is also useful with newer mkfs.btrfs; my fedora uses btrfs-progs v4.17.1 and this patch fix the issue so, 1589abb is okay.

@thehajime thehajime merged commit 76f5d3d into lkl:master Apr 25, 2019
@thehajime
Copy link
Member

@luckyluke for the follow up fix, could you please make a pull request to https://github.com/lkl/lkl-docker/commits/master ? if we could make the image, we may be able to pull the docker image from docker hub (lkldocker/).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support 32-bit builds

4 participants