Skip to content
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

Vxworks7 #592

Closed
wants to merge 3 commits into from
Closed

Vxworks7 #592

wants to merge 3 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Sep 4, 2020

Describe the contribution
This is changes for vxworks7, built on top of a standalone branch to solve several Wconversion warnings when compiling for the platform.

Testing performed
I built it and ran my environment with CF 3.0 on a PPC 5020 with vxWorks 7.

Expected behavior changes

None, but this branch suggests some small API typing changes.
System(s) tested on
PowerPC 5020 (dual-core 64-bit powerpc)
vxWorks 7

Additional context
There were some issues building base stuff with conversion and cast-align issues with the vxworks7 64-bit powerpc toolchain.

Contributor Info - All information REQUIRED for consideration of pull request
Steven Seeger, GSFC-582. Embedded Flight Systems, Inc.

Copy link
Contributor

@jphickey jphickey 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 like this is adding an entirely new dir for "vxworks7" which appears to be a clone of "vxworks" from a while back....

The changes don't look to be terribly substantial - certainly not enough to justify treating VxWorks7 as a completely different OS. In order to review I downloaded this PR locally and did a diff between "vxworks" and "vxworks7". Also the "5020-vxworks7" bsp appears largely identical to the "generic-vxworks" ... the point of generic-vxworks is that it can be used without cloning. It seems it should work here too, so we shouldn't clone unless there is a real need to do so.

Changes to the OS layer seem to be summarized as:

  • Update to GenericSemTake for when sys_ticks is 0 -- good change, probably a bug in 6.9 too
  • Update to use standard POSIX "mkdir" function -- probably can be done in CMake/source selection again candidate
  • Some 64-bit type related changes in queue and symtab code
  • Many typecasts sprinkled around elsewhere, which appear to be related to warnings triggered by -Wconversion

Each of the above should probably warrant a separate pull request and review, but each change should be made to the existing code (vxworks) rather than introducing a new copy of the code.

Note we should probably have a separate CCB discussion on -Wconversion --- this is not a warning we have enabled in the past due to its high false-positive rate. If we want to move toward also turning this warning on that's fine, but the frequent typecasting required to quelch these warnings tends to make the code harder to read. I don't really consider that part of "vxworks7" support either, aside from the fact that this uses clang which (maybe?) has different defaults for -Wall (which we do use), we can still turn it off with -Wno-conversion. At any rate I consider that a separate issue from VxWorks 7 support - Not saying don't merge them, just saying don't mix with VxWorks 7.

@jphickey
Copy link
Contributor

jphickey commented Sep 5, 2020

FYI the travis-CI failure being reported here for the coverage-vxworks-posix-io test is really a separate type issue that this exposed (signed vs unsigned), I fixed it in #594.

@ghost
Copy link
Author

ghost commented Sep 5, 2020

Hi Joe.

This was made a pull request to discuss, namely for the conversion branch changes it's based on. The conversion branch also contains stuff for -Wcast-align.

The reason I brought this up is -Wconversion and -Wcast-align and -Werror are all on by default in the vxworks7 toolchain's cmake toolchain file.

I was told that a pull request is how we can discuss the conversion things I ran across with the CCB. Or, as my preference would be, to turn this off and never look back. ;) -Wcast-align is worth fixing, though, and I think efforts are being made there.

The changes don't look to be terribly substantial - certainly not enough to justify treating VxWorks7 as a completely different OS. In order to review I downloaded this PR locally and did a diff between "vxworks" and "vxworks7". Also the "5020-vxworks7" bsp appears largely identical to the "generic-vxworks" ... the point of generic-vxworks is that it can be used without cloning. It seems it should work here too, so we shouldn't clone unless there is a real need to do so.

So the vxworks7 changes are probably not complete. A big change that needs to happen is we are relying on some pretty old legacy ramdisk stuff and I'd like to remove it completely.

It might be worth doing kind of what was done with the posix stuff where it's separated out into core vxworks and then we do 6 and 7 on top of that. That's a bit of work, but not impossible.

The current state of this was just to get it running.

Changes to the OS layer seem to be summarized as:

  • Update to GenericSemTake for when sys_ticks is 0 -- good change, probably a bug in 6.9 too
  • Update to use standard POSIX "mkdir" function -- probably can be done in CMake/source selection again candidate
  • Some 64-bit type related changes in queue and symtab code
  • Many typecasts sprinkled around elsewhere, which appear to be related to warnings triggered by -Wconversion

Each of the above should probably warrant a separate pull request and review, but each change should be made to the existing code (vxworks) rather than introducing a new copy of the code.

So how could we make these changes in the existing code? I'm open to suggestions. Do we just want #if guards around the differences?

Note we should probably have a separate CCB discussion on -Wconversion --- this is not a warning we have enabled in the past due to its high false-positive rate. If we want to move toward also turning this warning on that's fine, but the frequent typecasting required to quelch these warnings tends to make the code harder to read. I don't really consider that part of "vxworks7" support either, aside from the fact that this uses clang which (maybe?) has different defaults for -Wall (which we do use), we can still turn it off with -Wno-conversion. At any rate I consider that a separate issue from VxWorks 7 support - Not saying don't merge them, just saying don't mix with VxWorks 7.

Yes, the point of this pull request was to have this discussion on Wconversion. I was told to do this in the the CCB meeting before last.

I'm totally fine revamping this work to not add a whole new directory. I just wasn't sure what the group would support so I figured this was a good starting to point to show where I am.

@yammajamma yammajamma added CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) CCB-20200909 and removed CCB-20200909 labels Sep 9, 2020
@ghost ghost closed this Sep 16, 2020
@yammajamma yammajamma removed the CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) label Sep 16, 2020
@skliper
Copy link
Contributor

skliper commented Sep 21, 2020

Marked as duplicate of #599

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants