Skip to content

Conversation

@MartinNowak
Copy link
Member

  • as the purpose is really not testing call and symobl ABIs
  • always use -fPIC so we're compatible with PIE-by-default platforms

- as the purpose is really not testing calling and symobl ABIs
- always use -fPIC so we're compatible with PIE platforms
@dlang-bot
Copy link
Contributor

Thanks for your pull request, @MartinNowak!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

@JinShil
Copy link
Contributor

JinShil commented Jan 15, 2018

This appears to be trying to solve a non-existent problem. The test works for both PIC and non-PIC builds. If we pull this we lose the test on non-PIC builds and all the work and knowledge that went into creating it. I don't see that there's anything to "fix".

Furthermore, @MartinNowak, where were you a month ago when we were working on this and pinging you about it?

@wilzbach
Copy link
Contributor

How about we making the testsuite runnable on a PIE hardened system first?
See e.g. #7420

@MartinNowak
Copy link
Member Author

MartinNowak commented Jan 16, 2018

This appears to be trying to solve a non-existent problem. The test works for both PIC and non-PIC builds. If we pull this we lose the test on non-PIC builds and all the work and knowledge that went into creating it.

It's a redundant test that checks the same thing, b/c vector filling doesn't depend on PIC.
Hence it's confusing that we make this distinction here. Mainly the asm compare method is still flawed and contain the pre- and postambles of the test functions, that's what differs between the two.
The next person (or me) will just copy and paste the PIC stuff turning it into useless cargo cult.

I don't see that there's anything to "fix".

Check again, I've stripped PIC from the newly added test_cdstrpar because I couldn't see any sense in it. It is likely suffering from the same problem you were solving for test_cdvecfill.

@JinShil
Copy link
Contributor

JinShil commented Jan 16, 2018

How about we making the testsuite runnable on a PIE hardened system first?
See e.g. #7420

Exactly! I'll revisit this PR when we have the test suite running on PIE hardened systems and added to the CIs.

JinShil
JinShil previously approved these changes Jan 16, 2018
Copy link
Contributor

@JinShil JinShil left a comment

Choose a reason for hiding this comment

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

Ok, It finally dawned on me what @MartinNowak is saying with regard to the redundancy of the existing implementation. Therefore approving.

@JinShil
Copy link
Contributor

JinShil commented Jan 16, 2018

Auto-merge toggled off. Waiting for feedback from others on #7427 (comment)

@JinShil JinShil dismissed their stale review January 16, 2018 02:40

Dismissing review for now. Waiting for feedback on comments in #7427.

@dlang-bot dlang-bot merged commit cd6b40f into dlang:master Jan 16, 2018
@MartinNowak MartinNowak deleted the pic_asm branch February 7, 2018 10:33
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.

4 participants