Skip to content

[ARC] [libgloss] Introduce hostlink interface #14

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 4, 2021

Conversation

VVIsaev
Copy link
Contributor

@VVIsaev VVIsaev commented Jan 21, 2021

There is a special interface built in ARC simulators
(such as NSIM) called Metaware hostlink IO which can be used
to implement system calls.

This commit adds support for this interface to the ARC port of libgloss.

Copy link
Contributor

@claziss claziss left a comment

Choose a reason for hiding this comment

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

Header file are not ok, code style also. Please address my remarks and resubmit for a new round.

Copy link
Member

@shahab-vahedi shahab-vahedi left a comment

Choose a reason for hiding this comment

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

Good job on this Vladimir! Please find my remarks/questions all over the files.

Last but not least, I assume you ran GCC's DejaGnu test with this, right? Could you provide a link to the results with hostlink and without this hostlink so we can know the regressions?

EDIT
Vladimir has sent the results and his analysis. For future reference, I partially add it here:

Known regressions:

  • 2 more fails for binutils suite, this is ‘strip’ test. Metaware hostlink does not work with stripped binaries;
  • Lots of fails for libstdc++ testsuite – there are also more PASSes, this is because many unsupported tests become supported with Metaware hostlink;
  • 1 more fail for BE ARC newlib – this is because Metaware Extended IO for BE causes nsim to exit (there is code in nsim like if (ext_io hostlink and BE) then exit(FAIL);
  • Lots of fails for gdb testsuite – GDB nsim configuration is Jenkins-local, so all tests were for Metaware hostlink ran with incorrect nsim configuration.

Copy link
Member

@shahab-vahedi shahab-vahedi left a comment

Choose a reason for hiding this comment

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

Thank you for your rework. There are a few minor remarks that when are fixed, it should be good to go.

EDIT
If this library can be built separately, could you also mention in the documentation how to build it? Like:

cd libgloss/arc
configure blah
make blah blah
...

@abrodkin
Copy link
Member

Also, once we're done with this work, we need to add corresponding "baseboard" support here: https://github.com/foss-for-synopsys-dwc-arc-processors/toolchain/tree/arc-dev/dejagnu/baseboards so that we may integrate it easily into our verification flow.

@claziss
Copy link
Contributor

claziss commented Jan 30, 2021

@VVIsaev Please make sure u run check_GNU_style.py utility found in the gcc's contrib folder. It will flush your remaining code style issues.

@VVIsaev VVIsaev closed this Feb 1, 2021
@VVIsaev VVIsaev reopened this Feb 1, 2021
@VVIsaev
Copy link
Contributor Author

VVIsaev commented Feb 1, 2021

@claziss I ran check_GNU_style.py and now I think I fixed most of it comments. It does not like following comments (because of dot, space, space, end of comment rule):

  p = _hl_message (HL_SYSCALL_WRITE, "ipi:ii",
		   (uint32_t) fd,		/* i */
		   buf, (uint32_t) nbyte,	/* p */
		   (uint32_t) nbyte,		/* i */
		   (uint32_t *) &n,		/* :i */
		   (uint32_t *) &host_errno	/* :i */)

but I prefer to keep that comments. Please let me know if we need 0 errors from this script.

@shahab-vahedi for now this code is used only as a part of newlib. We think that it can be used in some more places, but it will require some changes.

@claziss
Copy link
Contributor

claziss commented Feb 1, 2021

@claziss I ran check_GNU_style.py and now I think I fixed most of it comments. It does not like following comments (because of dot, space, space, end of comment rule):

  p = _hl_message (HL_SYSCALL_WRITE, "ipi:ii",
		   (uint32_t) fd,		/* i */
		   buf, (uint32_t) nbyte,	/* p */
		   (uint32_t) nbyte,		/* i */
		   (uint32_t *) &n,		/* :i */
		   (uint32_t *) &host_errno	/* :i */)

but I prefer to keep that comments. Please let me know if we need 0 errors from this script.

@shahab-vahedi for now this code is used only as a part of newlib. We think that it can be used in some more places, but it will require some changes.

Probably you can have them there for the time being. Once you go upstream, the maintainer will tell you if he likes it or not.

@abrodkin
Copy link
Member

abrodkin commented Feb 3, 2021

@VVIsaev, @claziss, @shahab-vahedi is there anything that needs to be done here, or we may finally get this one merged and re-use it as a base of #13?

@shahab-vahedi
Copy link
Member

There are still unresolved issues.

@VVIsaev
Copy link
Contributor Author

VVIsaev commented Feb 3, 2021

I resolved remaining issues (about hl/configure because it is removed from last revision and about readme-hostlink.md).

Copy link
Contributor

@claziss claziss left a comment

Choose a reason for hiding this comment

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

It looks ok. Please document the difference in pass/failed tests for baremetal-dejagnu, if they still exists, in a github issue.

There is a special interface built in ARC simulators
(such as NSIM) called Metaware hostlink IO which can be used
to implement system calls.

This commit adds support for this interface to the ARC port of libgloss.
Copy link
Member

@shahab-vahedi shahab-vahedi left a comment

Choose a reason for hiding this comment

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

I've created an issue for the gettimeofday(). From my point of view, this patch looks good to me.

EDIT
That issue is resolved now.

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.

4 participants