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

Subdir Makefiles' compiler flags #5599

Merged

Conversation

magnumripper
Copy link
Member

@magnumripper magnumripper commented Dec 2, 2024

In separate commits:

  • Propagate CPU compiler flags to subdirs
  • Add -std=c99 globally, if compiler supports it
  • Add -maes -mpclmul if supported
  • Add them to legacy Makefile as well

Needs testing on non-intel, and more importantly on intel that doesn't support AES-NI

Legacy Makefiles not yet amended for AES-NI (will discuss in #5593)

@magnumripper
Copy link
Member Author

Interesting side effect of -std=c99

snefru_plug.c:784:11: error: call to undeclared function 'JOHNSWAP'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
                        W[4] = be2me_32(block[0]), W[5] = be2me_32(block[1]);
                               ^
./gost.h:74:22: note: expanded from macro 'be2me_32'
 #define be2me_32(x) JOHNSWAP(x)
                     ^

I take it johnswap.h isn't sourced... but then how could it work without -std=c99?

@magnumripper
Copy link
Member Author

I take it johnswap.h isn't sourced

But it is, and there seems to be a bug in it not surfaced in all these years until now. The logic ifdef chain doesn't end with a blind #else and I suppose none of the alternatives gets a hit.

#if defined __GNUC__ && ((__GNUC__ == 4 && __GNUC_MINOR__ >= 3) || (__GNUC__ > 4))
(...)
#elif (_MSC_VER > 1300) && (_M_IX86 >= 400 || defined(CPU_IA32) ||  defined(CPU_X64)) /* MS VC */
(...)
#elif !defined(__STRICT_ANSI__)
(...)
#endif

The failing bots run clang.

@solardiz
Copy link
Member

solardiz commented Dec 2, 2024

#if defined GNUC && ((GNUC == 4 && GNUC_MINOR >= 3) || (GNUC > 4))
The failing bots run clang.

Oh, clang sets these to indicate gcc 4.2.1 compatibility:

$ clang -dM -E - | fgrep GNUC
#define __GNUC_MINOR__ 2
#define __GNUC_PATCHLEVEL__ 1
#define __GNUC_STDC_INLINE__ 1
#define __GNUC__ 4

It's a near miss (or near hit?), which luckily uncovered the bug for us.

@magnumripper
Copy link
Member Author

If I change the #elif !defined(__STRICT_ANSI__) to just #else, it works fine. That is from b8e0ec3 in 2012.

@solardiz
Copy link
Member

solardiz commented Dec 2, 2024

Maybe the issue is this file had DOS linefeeds. Does the edited line possibly end in LF only?

Yes it had DOS LF so should never have been permitted into the repo. From what I can see, my editor added the line with CRLF (as the rest of the file was like that) but there may be some git crlf magic involved. I'll fixup with that whole file converted to proper line ending.

EDIT: Apparently I inadvertently edited this comment instead of replying to it /magnum

@magnumripper
Copy link
Member Author

If I change the #elif !defined(__STRICT_ANSI__) to just #else, it works fine. That is from b8e0ec3 in 2012.

Confirmed, adding -std=c99 to clang will result in __STRICT_ANSI__ being defined. I see from git history that this conditional originally came from gost.h and had an else clause. Then there were messy changes over the years.

I'll just drop it in favor of a plain #else.

@magnumripper magnumripper force-pushed the subdir-flags-and-aesni branch from b40df81 to 5059999 Compare December 2, 2024 03:18
@magnumripper
Copy link
Member Author

The addition of -std=c99 has strange results on super:

Configure finished.  Now "make -s clean && make -sj1" to compile.
/usr/bin/ar: creating poly1305-donna.a
In file included from pkzip.h:11,
                 from zip2john.c:137:
dyna_salt.h:45: warning: declaration does not declare anything
In file included from pkzip.h:11,
                 from pkzip.c:9:
dyna_salt.h:45: warning: declaration does not declare anything
In file included from 7z_common_plug.c:33:
dyna_salt.h:45: warning: declaration does not declare anything
In file included from 7z_fmt_plug.c:46:
dyna_salt.h:45: warning: declaration does not declare anything
pkzip.c: In function 'winzip_common_get_salt':
pkzip.c:219: error: 'dyna_salt' has no member named 'salt_alloc_needs_free'
pkzip.c:225: error: 'dyna_salt' has no member named 'salt_cmp_offset'
make[1]: *** [pkzip.o] Error 1
make[1]: *** Waiting for unfinished jobs....
7z_common_plug.c: In function 'sevenzip_get_salt':
7z_common_plug.c:311: error: 'dyna_salt' has no member named 'salt_cmp_offset'
7z_common_plug.c:313: error: 'dyna_salt' has no member named 'salt_alloc_needs_free'
make[1]: *** [7z_common_plug.o] Error 1
/usr/bin/ar: creating aes.a
/usr/bin/ar: creating ed25519-donna.a
/usr/bin/ar: creating secp256k1.a
make: *** [default] Error 2

Here's the offending code declaration:

typedef struct dyna_salt_t {
	size_t salt_cmp_size;
	struct { /* bit field stealing one bit of the size_t */
		size_t salt_alloc_needs_free : 1; /* 1 if if malloc/calloc used */
		size_t salt_cmp_offset : (sizeof(size_t) * 8 - 1);
	};
} dyna_salt;

@magnumripper
Copy link
Member Author

magnumripper commented Dec 2, 2024

I am stumped. I changed the confusing struct/typedef names to this [because in other places dyna_salt was used as typedef name, struct name, member name and variable name], but it did not help

typedef struct dyna_salt_s {
	size_t salt_cmp_size;
	struct { /* bit field stealing one bit of the size_t */
		size_t salt_alloc_needs_free : 1; /* 1 if if malloc/calloc used */
		size_t salt_cmp_offset : (sizeof(size_t) * 8 - 1);
	};
} dyna_salt_t;

Giving up for now, need sleep

@magnumripper
Copy link
Member Author

magnumripper commented Dec 2, 2024

Oh, apparently you're not allowed to use size_t for bitfields. EDIT: that was not it - but using size_t is apparently not in any standard but a GNUism.

@magnumripper magnumripper force-pushed the subdir-flags-and-aesni branch from cb36a14 to ff85f01 Compare December 2, 2024 05:00
@magnumripper
Copy link
Member Author

magnumripper commented Dec 2, 2024

I'm dropping that C99 thing for now, that'll be a later PR. There are more problems with it. It's about anonymous unions/structs. Maybe -std=gnu99 is an option, maybe not.

@solardiz
Copy link
Member

solardiz commented Dec 2, 2024

Configure finished. Now "make -s clean && make -sj1" to compile.

Unrelated to this PR, but I notice this says -sj1. Do we have an issue with CPU count detection? I think I saw it correctly say -sj32 on that machine.

@solardiz
Copy link
Member

solardiz commented Dec 2, 2024

Yes it had DOS LF so should never have been permitted into the repo.

Right, but this predates our whitespace-errors CI check. A lot of the ZTEX files are like that. This is part of why in that check we specify a certain git revision to start checking from - I couldn't easily fix all files at the time of introduction of that check.

I'll fixup with that whole file converted to proper line ending.

Sounds good, but ideally that would be a separate commit just before your commit that makes the actual change.

Copy link
Member

@solardiz solardiz left a comment

Choose a reason for hiding this comment

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

Let's get this in first and then revisit the C99 stuff separately.

@magnumripper
Copy link
Member Author

magnumripper commented Dec 3, 2024

Configure finished. Now "make -s clean && make -sj1" to compile.

Unrelated to this PR, but I notice this says -sj1. Do we have an issue with CPU count detection? I think I saw it correctly say -sj32 on that machine.

Not sure where you saw that but if it was something I posted, I probably had OMP_NUM_THREADS=1 exported (if that matters?) and I ususally do nice make -sj on that one anyway.

@magnumripper magnumripper force-pushed the subdir-flags-and-aesni branch from ff85f01 to d9530bd Compare December 3, 2024 09:21
@magnumripper magnumripper merged commit 94b2144 into openwall:bleeding-jumbo Dec 3, 2024
28 of 34 checks passed
@magnumripper magnumripper deleted the subdir-flags-and-aesni branch December 3, 2024 09:22
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