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

Add Make.win32 -fcommon, define u64int, sync libsec, libmp, libauthsrv with Plan 9, define usize, add chacha20, libsec: add mising \ #16

Merged
merged 8 commits into from
Jun 24, 2024

Conversation

adriangrigore
Copy link
Contributor

No description provided.

@0intro
Copy link
Member

0intro commented Jun 22, 2024

Thanks. I'm looking at it.

@adriangrigore adriangrigore changed the title Add -fcommon, define u64int, sync libsec with Plan 9 Add -fcommon, define u64int, sync libsec, libmp with Plan 9 Jun 23, 2024
@adriangrigore adriangrigore changed the title Add -fcommon, define u64int, sync libsec, libmp with Plan 9 Add -fcommon, define u64int, sync libsec, libmp, libauthsrv with Plan 9 Jun 23, 2024
@adriangrigore
Copy link
Contributor Author

I removed #pragmas at start of header files, maybe it's a good idea to leave them there despite the warnings. Proper solution I think would be to #ifdef them: https://github.com/9fans/drawterm/blob/main/include/authsrv.h but that would mean patching on Plan 9 side also.

@adriangrigore
Copy link
Contributor Author

adriangrigore commented Jun 23, 2024

Oh, I see there are more changes in this repo vs Plan 9 versions(not that many) I just copied the libs.

I could merge and force push I think.

@adriangrigore adriangrigore changed the title Add -fcommon, define u64int, sync libsec, libmp, libauthsrv with Plan 9 Add -fcommon, define u64int, sync libsec, libmp, libauthsrv with Plan 9, define usize, add chacha20 Jun 23, 2024
@0intro
Copy link
Member

0intro commented Jun 23, 2024

Overall, it looks good. Thank your for working on this.

Could you rename libsec: sync with Plan9 to libsec: sync with Plan 9?

@adriangrigore adriangrigore changed the title Add -fcommon, define u64int, sync libsec, libmp, libauthsrv with Plan 9, define usize, add chacha20 Add -fcommon, -Wno-unknown-pragmas, define u64int, sync libsec, libmp, libauthsrv with Plan 9, define usize, add chacha20 Jun 23, 2024
@adriangrigore adriangrigore changed the title Add -fcommon, -Wno-unknown-pragmas, define u64int, sync libsec, libmp, libauthsrv with Plan 9, define usize, add chacha20 Add Make.win32 -fcommon, -Wno-unknown-pragmas, define u64int, sync libsec, libmp, libauthsrv with Plan 9, define usize, add chacha20 Jun 23, 2024
@adriangrigore
Copy link
Contributor Author

adriangrigore commented Jun 23, 2024

Ok, changed libsec commit message, left #pragmas in place and added -Wno-unknown-pragmas to Make.win32.

@0intro
Copy link
Member

0intro commented Jun 23, 2024

I think it's probably better to add #ifdef PLAN9 around pragma, like it was done for the other header files (include/draw.h and so on).

@adriangrigore
Copy link
Contributor Author

I took the flag from here https://github.com/golang/go/blob/ceb95ea6aef52c1fb472d3539c6ef68670778b5b/src/cmd/dist/build.c#L482, not sure if it applies to Plan 9, and I think it's preferable to have identical copies in drawterm and Plan 9. If I were to add #ifdefs, I'd probably add them to the Plan 9 versions also to keep them precisely in sync. That's why I opted for -Wno-unknown-pragmas.

@adriangrigore
Copy link
Contributor Author

adriangrigore commented Jun 23, 2024

plan9port removes them completely https://github.com/9fans/plan9port/blob/master/include/mp.h#L137 or comments them out https://github.com/9fans/plan9port/blob/master/include/mp.h#L151, I understand your argument of #idefing them to keep consistency with the current codebase, I would agree with that for consistency purposes.

@adriangrigore
Copy link
Contributor Author

Shall we go with consistency then?

@adriangrigore
Copy link
Contributor Author

Although, not all them were #ifdef-ed, some just had their #pragmas removed https://github.com/9fans/drawterm/blob/main/include/libsec.h, https://github.com/9fans/drawterm/blob/main/include/mp.h.

@adriangrigore
Copy link
Contributor Author

If you prefer #ifdefs, 9legacy solution would probably be create some patches to ifdef them in Plan 9 too I think. To keep them perfectly in sync, wih no changes to the Plan 9 versions, -Wno-unknown-pragmas would probably be the solution.

@adriangrigore
Copy link
Contributor Author

Or just live with the warnings.

@0intro
Copy link
Member

0intro commented Jun 23, 2024

The pragma are meant to be interpreted by the Plan 9 C compiler only. I'm not very comfortable keeping them, risking to be handled by the GCC preprocessor, which could lead to side effects.

Either deleting them or #ifdef sounds good to me.

@adriangrigore
Copy link
Contributor Author

The pragma are meant to be interpreted by the Plan 9 C compiler only. I'm not very comfortable keeping them, risking to be handled by the GCC preprocessor, which could lead to side effects.

Either deleting them or #ifdef sounds good to me.

Ok, I'll probably #ifdef them then.

@adriangrigore adriangrigore changed the title Add Make.win32 -fcommon, -Wno-unknown-pragmas, define u64int, sync libsec, libmp, libauthsrv with Plan 9, define usize, add chacha20 Add Make.win32 -fcommon, define u64int, sync libsec, libmp, libauthsrv with Plan 9, define usize, add chacha20 Jun 23, 2024
@adriangrigore
Copy link
Contributor Author

Ok, #ifdef-ed, not sure if this is ok 0f4e314#diff-e9399bd85711a4c93d1a01968311639e3355a8b3dce252f026d44cd7650ae913R130, if ok, I think you can merge.

@adriangrigore
Copy link
Contributor Author

Hmm... this might require merging 93a43be

@adriangrigore
Copy link
Contributor Author

I don't understand which (if any of the patches) to libmp, libsec, libauthsrv from this repo have been merged in Plan 9 libs, if not, we'll have to merge them.

@adriangrigore
Copy link
Contributor Author

Hmm... this might require merging 93a43be

I see block files are done in assembly, I added the C versions 15a79f4#diff-ede5614f90d7b8d84cda78003b911a92e627073420f71965b843c0c4bbab4921R50 for sha2block64 and sha2block128.

@adriangrigore
Copy link
Contributor Author

@adriangrigore
Copy link
Contributor Author

adriangrigore commented Jun 23, 2024

6fb8584 and e10503f look merged.

@adriangrigore
Copy link
Contributor Author

@adriangrigore
Copy link
Contributor Author

@0intro
Copy link
Member

0intro commented Jun 23, 2024

I don't understand which (if any of the patches) to libmp, libsec, libauthsrv from this repo have been merged in Plan 9 libs, if not, we'll have to merge them.

Merging is always complicated because you have to compare two pieces of code that evolved independently.

@adriangrigore
Copy link
Contributor Author

I don't understand which (if any of the patches) to libmp, libsec, libauthsrv from this repo have been merged in Plan 9 libs, if not, we'll have to merge them.

Merging is always complicated because you have to compare two pieces of code that evolved independently.

I assume they started from the same sources, and I assume the idea was for them to be sync-ed, if so, they don't seem to have a lot of changes between them. Please take a look at libsec, libmp and libauthsrv commit history in this repo, there's not a lof of changes. Doesn't seem hard to merge. Changes are minor.

@0intro
Copy link
Member

0intro commented Jun 23, 2024

1125f09#diff-ba43ca4048f51025813c140fdcc7f53f77817dd416f1aa445900c524e5090b86L1 affects libmp, not merged.

For that kind of changes, you have to re-apply the changes Russ did on your libmp: sync with Plan 9 commit to be sure there are no regression.

@adriangrigore
Copy link
Contributor Author

1125f09#diff-ba43ca4048f51025813c140fdcc7f53f77817dd416f1aa445900c524e5090b86L1 affects libmp, not merged.

For that kind of changes, you have to re-apply the changes Russ did on your libmp: sync with Plan 9 commit to be sure there are no regression.

Yup, exactly! Doesn't seem hard! :)

@adriangrigore
Copy link
Contributor Author

Ok ,merged, patches for 9legacy if interested.

libmp-gcc-warnings-drawterm.diff

diff --git a/libmp/strtomp.c b/libmp/strtomp.c
index 9c64c33..26e3751 100644
--- a/libmp/strtomp.c
+++ b/libmp/strtomp.c
@@ -32,13 +32,13 @@ init(void)
 	memset(tab.t10, INVAL, sizeof(tab.t10));
 
 	for(p = set64; *p; p++)
-		tab.t64[*p] = p-set64;
+		tab.t64[(uchar)*p] = p-set64;
 	for(p = set32; *p; p++)
-		tab.t32[*p] = p-set32;
+		tab.t32[(uchar)*p] = p-set32;
 	for(p = set16; *p; p++)
-		tab.t16[*p] = (p-set16)%16;
+		tab.t16[(uchar)*p] = (p-set16)%16;
 	for(p = set10; *p; p++)
-		tab.t10[*p] = (p-set10);
+		tab.t10[(uchar)*p] = (p-set10);
 
 	tab.inited = 1;
 }

libmp-sun-fixes-drawterm.diff

diff --git a/libmp/dat.h b/libmp/dat.h
index 7c834ac..8542909 100644
--- a/libmp/dat.h
+++ b/libmp/dat.h
@@ -1,4 +1,4 @@
-#define	mpdighi  (mpdigit)(1<<(Dbits-1))
+#define	mpdighi  (mpdigit)(1U<<(Dbits-1))
 #define DIGITS(x) ((Dbits - 1 + (x))/Dbits)
 
 // for converting between int's and mpint's

Now, I really don't want to write sha2block64 and sha2block128 in assembler now. :D

@adriangrigore
Copy link
Contributor Author

adriangrigore commented Jun 23, 2024

Ok, #ifdef-ed, not sure if this is ok 0f4e314#diff-e9399bd85711a4c93d1a01968311639e3355a8b3dce252f026d44cd7650ae913R130, if ok, I think you can merge.

Please review this also, if I didn't miss anything when merging repo changes (I don't think so), it's fine to merge now, except for the asm version of sha2block64 and sha2block128 which I don't think affects us at all.

@0intro
Copy link
Member

0intro commented Jun 23, 2024

Thanks. I'll will do a final review.

@adriangrigore adriangrigore changed the title Add Make.win32 -fcommon, define u64int, sync libsec, libmp, libauthsrv with Plan 9, define usize, add chacha20 Add Make.win32 -fcommon, define u64int, sync libsec, libmp, libauthsrv with Plan 9, define usize, add chacha20, libsec: add mising \ Jun 23, 2024
@adriangrigore
Copy link
Contributor Author

Perhaps you might consider merging to a separate branch for now until everything is complete?

Copy link
Member

@0intro 0intro left a comment

Choose a reason for hiding this comment

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

I think your changes are ready to be merged on the main branch.
You will be able to open new PRs for the remaining changes.

@@ -51,7 +51,7 @@ OFILES=\
sha2block64.$O\
sha2_128.$O\
sha2block128.$O\
smallprimes.$O
smallprimes.$O\
Copy link
Member

Choose a reason for hiding this comment

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

This is not necessary, but it doesn't harm.

Copy link
Contributor Author

@adriangrigore adriangrigore Jun 24, 2024

Choose a reason for hiding this comment

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

I guess I preferred single line diffs as I have to add more stuff there. Your call!

@0intro
Copy link
Member

0intro commented Jun 24, 2024

I'm ready to merge into main. Let me know if it's good for you.

@adriangrigore
Copy link
Contributor Author

Ok to merge!

@0intro 0intro merged commit ba4a812 into 9fans:main Jun 24, 2024
1 check passed
@adriangrigore
Copy link
Contributor Author

I think this also closes #12

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.

2 participants