Skip to content

Conversation

@kripken
Copy link
Member

@kripken kripken commented May 11, 2020

Add a libc variation that supports full long double precision printing.
It turns out that adding this is not that hard, building on the existing
work we've already done there.

One thing I dislike here is that an additional libc variation means we
double the number of libc builds, and the size of the libc builds in
the emsdk, I believe? I wonder if maybe this should not be built by
embuilder by default or something like that.

@kripken kripken requested a review from sbc100 May 11, 2020 21:04
@sbc100
Copy link
Collaborator

sbc100 commented May 11, 2020

One option would be to make a separate libc-long-double.a that gets included on the link line before libc itself when the option is enabled.

@kripken
Copy link
Member Author

kripken commented May 11, 2020

How would that get built @sbc100 ?

@sbc100
Copy link
Collaborator

sbc100 commented May 11, 2020

It could be a new library just includes libc/musl/src/stdio/vfprintf.c and always is built with long double support.

Then the main libc could be unchanged/shared.

@kripken
Copy link
Member Author

kripken commented May 11, 2020

I see, thanks @sbc100 - rewritten to be that way now, much better I think!


# to override the normal libc printf, we must come before it
if shared.Settings.PRINTF_LONG_DOUBLE:
libs_to_link = [(system_libs_map['libprintf_long_double'].get_path(), True)] + libs_to_link
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you can, I would put this on line 1577 right before libc (using the same add_library helper)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess that same goes for jsmath too?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't follow? Line 1577 is

        for haz in has_syms:
          if haz in need_syms:
            # remove symbols that are supplied by another of the inputs
            need_syms.remove(haz) # This is 1577..?
        if shared.Settings.VERBOSE:
          logger.debug('considering %s: we need %s and have %s' % (lib.name, str(need_syms), str(has_syms)))

So I'm not sure where you are suggesting to put this. But maybe the bigger issue is that this can't be moved earlier - it has to be after the sort operation on line 1641.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe you need to sync to head?

This line should come just before: add_library(system_libs_map['libc']).. order is important for all of the add_library function here.

The sort operations are are only for specific cases.. they won't apply to libc or this library (I will try to remove them.. they are porbably not needed anymore since that most recent changes here).

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, done.

(Oh, I assumed when you said line numbers they were relative to this PR... otherwise which HEAD would I look at..? it can keep changing outside. Is there a convention I'm not aware of?)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just just been lazy to looking at my local line numbers... Your are right its not a very accurate way to communicate file positions :)

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

Nice!

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

Yay!

@kripken kripken merged commit 9c8beb9 into master May 12, 2020
@kripken kripken deleted the pld branch May 12, 2020 14:52
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.

3 participants