Fix build errors on djgpp #19274

jwt27 · 2022-09-25T18:44:35Z

I tried cross-compiling the current master branch for djgpp and ran into some
errors. This patch set aims to solve those.

On each commit I specified CLA: trivial, since they are small tweaks and do
not introduce any new features. Only build errors are fixed.

Remaining issues:

test/rsa_complex.c does not compile due to missing header <complex.h>.
Linking is only possible when compiled with -march=i486 or better, due to
missing atomic instructions on i386 (these are compiled to libatomic calls,
which are also missing). Is it safe to define -DOPENSSL_DEV_NO_ATOMICS
here, given that we have no (native) pre-emptive threads?

Build failed on djgpp due to missing config vars 'AR' and 'ARFLAGS'. Additionally, '-lz' was not added to 'lflags' when zlib support was enabled. Inheriting configuration variables from BASE_unix solves both these issues. CLA: trivial

This part failed to compile due to a circular dependency between internal/e_os.h and internal/time.h, when ossl_sleep() falls back to a busy wait. However, djgpp has a usleep function, so it can use the regular Unix version of ossl_sleep(). It's not great though. The resolution is only ~55ms, and it may break when a user program hooks the timer interrupt without periodically updating BIOS time. A high-resolution alternative is uclock(), but that is generally less desirable since it reprograms the system timer. The circular dependency is still there and may still cause trouble for other platforms. CLA: trivial

If this macro is left undefined, Watt-32 will "helpfully" declare some typedefs such as 'byte' and 'word' in the global namespace. This broke compilation of apps/s_client.c. CLA: trivial

levitte · 2022-09-26T01:35:18Z

I marked this for the 3.0 branch, 'cause I think of this as bug fixes and that they should thus be backported. I'm unsure, though, if this applies cleanly on that branch, it's possible that a separate PR is needed.

It may be that the same reasoning should be made for 1.1.1.

Is this something you're willing to look into, @jwt27?

mattcaswell · 2022-09-26T09:35:16Z

It may be that the same reasoning should be made for 1.1.1.

1.1.1 is receiving security fixes only - so this would not be backported there.

t8m

Unfortunately at least the last commit is outside of what we would accept with CLA: trivial. Can you please sign the regular CLA? https://www.openssl.org/policies/cla.html

jwt27 · 2022-09-26T16:21:19Z

I marked this for the 3.0 branch, 'cause I think of this as bug fixes and that they should thus be backported. I'm unsure, though, if this applies cleanly on that branch, it's possible that a separate PR is needed.

It may be that the same reasoning should be made for 1.1.1.

Is this something you're willing to look into, @jwt27?

Sure, I had a look:

On openssl-3.0, the first and third patches apply cleanly. The other two are
not needed.

On OpenSSL_1_1_1-stable, only the third patch applies. AR/ARFLAGS are
correctly set on configure, however -lz is missing from the linker flags.
Inheriting config from BASE_unix does resolve this, but the respective patch
would need to be edited.

The remaining issues listed in my first post also apply to these branches.

Unfortunately at least the last commit is outside of what we would accept with CLA: trivial. Can you please sign the regular CLA? https://www.openssl.org/policies/cla.html

Alright, will do. I just realized using gmtime_r() produces the wrong
result, so I'll have to rewrite that patch anyway.

crypto/asn1/a_time.c

Changed since

Since djgpp has neither a timezone variable or timegm(), this horrible method must be used. It is the only one I could find that produces accurate results, and is recommended as portable alternative to timegm() by the GNU libc manual. Reference: https://www.gnu.org/software/libc/manual/html_node/Broken_002ddown-Time.html#index-timegm Now, a much nicer alternative solution could be: timestamp_local = mktime(timestamp_tm); timestamp_utc = timestamp_local + timestamp_tm->tm_gmtoff - (timestamp_tm->tm_isdst ? 3600 : 0); This works due to the fact that mktime() populates the tm_gmtoff and tm_isdst fields in the source timestamp. It is accurate everywhere in the world, *except* on Lord Howe Island, Australia, where a 30 minute DST offset is used.

jwt27 · 2022-09-27T14:58:31Z

CLA is signed, so this PR is ready for review. I was told I had to close and
reopen it to remove a "needs CLA" tag, but it was never added so I suppose
that's not necessary.

jwt27 · 2022-09-27T19:01:44Z

Another question - would it be useful to add djgpp as a CI build target? I
maintain an Ubuntu PPA specifically for that purpose. In fact, I ran into
these build errors as I was planning to add an OpenSSL package to it.

If there is any interest, and there are no objections to adding a dependency on
some random person's PPA, I can submit a PR to add it in cross-compiles.yml.

openssl-machine · 2022-09-28T18:00:14Z

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

Build failed on djgpp due to missing config vars 'AR' and 'ARFLAGS'. Additionally, '-lz' was not added to 'lflags' when zlib support was enabled. Inheriting configuration variables from BASE_unix solves both these issues. CLA: trivial Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #19274)

This part failed to compile due to a circular dependency between internal/e_os.h and internal/time.h, when ossl_sleep() falls back to a busy wait. However, djgpp has a usleep function, so it can use the regular Unix version of ossl_sleep(). It's not great though. The resolution is only ~55ms, and it may break when a user program hooks the timer interrupt without periodically updating BIOS time. A high-resolution alternative is uclock(), but that is generally less desirable since it reprograms the system timer. The circular dependency is still there and may still cause trouble for other platforms. CLA: trivial Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #19274)

If this macro is left undefined, Watt-32 will "helpfully" declare some typedefs such as 'byte' and 'word' in the global namespace. This broke compilation of apps/s_client.c. CLA: trivial Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #19274)

Since djgpp has neither a timezone variable or timegm(), this horrible method must be used. It is the only one I could find that produces accurate results, and is recommended as portable alternative to timegm() by the GNU libc manual. Reference: https://www.gnu.org/software/libc/manual/html_node/Broken_002ddown-Time.html#index-timegm Now, a much nicer alternative solution could be: timestamp_local = mktime(timestamp_tm); timestamp_utc = timestamp_local + timestamp_tm->tm_gmtoff - (timestamp_tm->tm_isdst ? 3600 : 0); This works due to the fact that mktime() populates the tm_gmtoff and tm_isdst fields in the source timestamp. It is accurate everywhere in the world, *except* on Lord Howe Island, Australia, where a 30 minute DST offset is used. Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #19274)

Build failed on djgpp due to missing config vars 'AR' and 'ARFLAGS'. Additionally, '-lz' was not added to 'lflags' when zlib support was enabled. Inheriting configuration variables from BASE_unix solves both these issues. CLA: trivial Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #19274) (cherry picked from commit 29d82bd)

This part failed to compile due to a circular dependency between internal/e_os.h and internal/time.h, when ossl_sleep() falls back to a busy wait. However, djgpp has a usleep function, so it can use the regular Unix version of ossl_sleep(). It's not great though. The resolution is only ~55ms, and it may break when a user program hooks the timer interrupt without periodically updating BIOS time. A high-resolution alternative is uclock(), but that is generally less desirable since it reprograms the system timer. The circular dependency is still there and may still cause trouble for other platforms. CLA: trivial Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #19274) (cherry picked from commit 6512559)

If this macro is left undefined, Watt-32 will "helpfully" declare some typedefs such as 'byte' and 'word' in the global namespace. This broke compilation of apps/s_client.c. CLA: trivial Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #19274) (cherry picked from commit 8ae74c5)

t8m · 2022-09-29T10:06:38Z

Merged to master branch. Cherry-picked to 3.0 branch except for the last commit which does not apply there.

Closing.

If there is any interest, and there are no objections to adding a dependency on some random person's PPA, I can submit a PR to add it in cross-compiles.yml.

I am not sure about that. You can submit a PR if you want but this is something we would need to discuss within OTC IMO before merging it.

jwt27 · 2022-09-29T19:43:26Z

Thanks for merging! I see now e_os.h is in a different location on the 3.0 branch, didn't notice that at first.

If there is any interest, and there are no objections to adding a dependency on some random person's PPA, I can submit a PR to add it in cross-compiles.yml.

I am not sure about that. You can submit a PR if you want but this is something we would need to discuss within OTC IMO before merging it.

That is understandable. I'll submit the PR then and see what happens.

Build failed on djgpp due to missing config vars 'AR' and 'ARFLAGS'. Additionally, '-lz' was not added to 'lflags' when zlib support was enabled. Inheriting configuration variables from BASE_unix solves both these issues. CLA: trivial Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#19274)

This part failed to compile due to a circular dependency between internal/e_os.h and internal/time.h, when ossl_sleep() falls back to a busy wait. However, djgpp has a usleep function, so it can use the regular Unix version of ossl_sleep(). It's not great though. The resolution is only ~55ms, and it may break when a user program hooks the timer interrupt without periodically updating BIOS time. A high-resolution alternative is uclock(), but that is generally less desirable since it reprograms the system timer. The circular dependency is still there and may still cause trouble for other platforms. CLA: trivial Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#19274)

If this macro is left undefined, Watt-32 will "helpfully" declare some typedefs such as 'byte' and 'word' in the global namespace. This broke compilation of apps/s_client.c. CLA: trivial Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#19274)

Since djgpp has neither a timezone variable or timegm(), this horrible method must be used. It is the only one I could find that produces accurate results, and is recommended as portable alternative to timegm() by the GNU libc manual. Reference: https://www.gnu.org/software/libc/manual/html_node/Broken_002ddown-Time.html#index-timegm Now, a much nicer alternative solution could be: timestamp_local = mktime(timestamp_tm); timestamp_utc = timestamp_local + timestamp_tm->tm_gmtoff - (timestamp_tm->tm_isdst ? 3600 : 0); This works due to the fact that mktime() populates the tm_gmtoff and tm_isdst fields in the source timestamp. It is accurate everywhere in the world, *except* on Lord Howe Island, Australia, where a 30 minute DST offset is used. Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#19274)

jwt27 added 3 commits September 25, 2022 19:04

djgpp: Inherit config from BASE_unix

b1cca45

Build failed on djgpp due to missing config vars 'AR' and 'ARFLAGS'. Additionally, '-lz' was not added to 'lflags' when zlib support was enabled. Inheriting configuration variables from BASE_unix solves both these issues. CLA: trivial

djgpp: Define WATT32_NO_OLDIES before including socket headers

091a9d7

If this macro is left undefined, Watt-32 will "helpfully" declare some typedefs such as 'byte' and 'word' in the global namespace. This broke compilation of apps/s_client.c. CLA: trivial

jwt27 marked this pull request as ready for review September 26, 2022 00:35

github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Sep 26, 2022

levitte previously approved these changes Sep 26, 2022

View reviewed changes

levitte added branch: master Merge to master branch approval: review pending This pull request needs review by a committer branch: 3.0 Merge to openssl-3.0 branch labels Sep 26, 2022

t8m added triaged: bug The issue/pr is/fixes a bug cla: trivial One of the commits is marked as 'CLA: trivial' labels Sep 26, 2022

t8m reviewed Sep 26, 2022

View reviewed changes

jwt27 marked this pull request as draft September 26, 2022 16:16

jwt27 force-pushed the djgpp branch from ecc7404 to 94469a9 Compare September 26, 2022 18:39

jwt27 marked this pull request as ready for review September 26, 2022 18:40

jwt27 force-pushed the djgpp branch from 94469a9 to bcc619f Compare September 26, 2022 18:49

t8m reviewed Sep 27, 2022

View reviewed changes

crypto/asn1/a_time.c Outdated Show resolved Hide resolved

t8m added approval: otc review pending and removed cla: trivial One of the commits is marked as 'CLA: trivial' labels Sep 27, 2022

jwt27 force-pushed the djgpp branch from bcc619f to c26d7bf Compare September 27, 2022 14:53

t8m approved these changes Sep 27, 2022

View reviewed changes

t8m removed the approval: otc review pending label Sep 27, 2022

t8m requested a review from levitte September 27, 2022 15:16

jwt27 mentioned this pull request Sep 27, 2022

Skip test/rsa_complex.c on djgpp #19286

Closed

1 task

levitte approved these changes Sep 27, 2022

View reviewed changes

levitte added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Sep 27, 2022

t8m added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Sep 29, 2022

t8m closed this Sep 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix build errors on djgpp #19274

Fix build errors on djgpp #19274

jwt27 commented Sep 25, 2022 •

edited

Loading

levitte commented Sep 26, 2022

mattcaswell commented Sep 26, 2022

t8m left a comment

jwt27 commented Sep 26, 2022

jwt27 commented Sep 27, 2022

jwt27 commented Sep 27, 2022

openssl-machine commented Sep 28, 2022

t8m commented Sep 29, 2022

jwt27 commented Sep 29, 2022

Fix build errors on djgpp #19274

Fix build errors on djgpp #19274

Conversation

jwt27 commented Sep 25, 2022 • edited Loading

levitte commented Sep 26, 2022

mattcaswell commented Sep 26, 2022

t8m left a comment

Choose a reason for hiding this comment

jwt27 commented Sep 26, 2022

jwt27 commented Sep 27, 2022

jwt27 commented Sep 27, 2022

openssl-machine commented Sep 28, 2022

t8m commented Sep 29, 2022

jwt27 commented Sep 29, 2022

jwt27 commented Sep 25, 2022 •

edited

Loading