Info on changes needed for building on Apple Silicon

[cho-m]

Describe the bug

Build error on Apple Silicon from pointer alignment in packing and LuaJIT flags.

ld: warning: pointer not aligned at address 0x10017E21D (_g_event_info + 527453 from ../../libscap/libscap.a(event_table.c.o))
ld: unaligned pointer(s) for architecture arm64

How to reproduce it

Was seen trying to build sysdig on Apple Silicon in Homebrew (https://github.com/Homebrew/homebrew-core/pull/95846)

Expected behaviour

Successful build.

Screenshots

N/A

Environment

• Falco version:
• System info:
• Cloud provider or hardware configuration: Apple Silicon / M1 (arm64)
• OS: macOS 11 and 12
• Kernel: Darwin
• Installation method: source via Homebrew

Additional context

For packing issue, problem was triaged to be due to ppm_param_info starting on 4-byte boundary and causing unaligned pointers (https://github.com/Homebrew/homebrew-core/pull/95846#issuecomment-1055022721). Temporary workaround in Homebrew was to disable packing on Apple Silicon, but should probably have proper fix by adjusting struct fields to fix alignment:

https://github.com/falcosecurity/libs/blob/baa4676a1f6b1dde1f02a0c7962fd6d0e33c78a0/driver/ppm_events_public.h#L1616-L1622

For LuaJIT flags, the following are not compatible with arm64. Also, in case of LuaJIT 2.1, they are no longer needed. https://github.com/falcosecurity/libs/blob/baa4676a1f6b1dde1f02a0c7962fd6d0e33c78a0/cmake/modules/CompilerFlags.cmake#L79-L81

[FedeDP]

Hi @cho-m! Thanks for taking the time to analyze this issue! Would you like to open a PR to fix these issues?

but should probably have proper fix by adjusting struct fields to fix alignment:

That is not possible without losing backward compatibility with existing scap files; we can't do that unfortunately :( Therefore the ifdef solution would be the best choice IMO.

[Andreagit97]

Thank you for opening this issue @cho-m! :rocket: I completely agree with @FedeDP, the ifdef seems the best solution

[FedeDP]

On second thought, using the ifdef would mean that arm64 won't be able to read scap files generated by amd64, and viceversa. I am not sure this is what we really want.

Weird thing is, we are currently in the process of officially supporting arm64 in Falco, and we never saw this issue. We have libs CI too that tests builds on arm64, but again, never saw these failures. Therefore, i am not sure how to proceed.

For LuaJIT flags, the following are not compatible with arm64. Also, in case of LuaJIT 2.1, they are no longer needed.

Can you share a link? Thank you!

/cc @leogr @ldegio

[FedeDP]

I think we could fix the unaligned issue by eg:

• adding a new ppm_event_flags like EF_NO_UNALIGNED_ACCESS
• swapping nparams with params (fixing alignment)
• keeping backward compatibility reading scap files: if the event as the EF_NO_UNALIGNED_ACCESS flag, we will read params first, then nparams; otherwise, we will read nparams then params.
• we would lose forward-compatibility reading a new scap file from an old version though

[cho-m]

Can you share a link?

https://github.com/LuaJIT/LuaJIT/issues/649#issuecomment-751341864

Also, website (https://luajit.org/install.html) was updated with linker flags removed and to specifically use 2.1 on macOS

Old version of website (https://web.archive.org/web/20220407012421/https://luajit.org/install.html) mentions difference between 2.0 and 2.1

Important: this relates to LuaJIT 2.0 only — use LuaJIT 2.1 to avoid these complications.

[FedeDP]

Thanks for the link! Are you willing to open a first PR to fix the luaJIT flags? :)

[poiana]

Issues go stale after 90d of inactivity.

Mark the issue as fresh with /remove-lifecycle stale.

Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Provide feedback via https://github.com/falcosecurity/community.

/lifecycle stale

[geraldcombs]

I see similar alignment errors here on macOS 12.5 x86_64.

ld: warning: pointer not aligned at address 0x1C0C2B (_g_event_info + 598283 from /opt/falco-libs-0.8.0-206-gff1f9805/lib/falcosecurity/libscap_event_schema.a(event_table.c.o))
[ ... ]
ld: warning: pointer not aligned at address 0x12F83D (_g_event_info + 3357 from /opt/falco-libs-0.8.0-206-gff1f9805/lib/falcosecurity/libscap_event_schema.a(event_table.c.o))
ld: unaligned pointer(s) for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

Assuming that this section of code describes the behavior of Xcode's "ld"...

https://github.com/apple-opensource/ld64/blob/e28c028b20/src/ld/OutputFile.cpp#L5411

...it looks like pointers need to be aligned to 64 bits on Arm64 and 32 bits on x86_64.

I added a bunch of debugging printfs to sinsp-example and it printed the following:

$ sinsp-example &g_event_info[0].name 0x10ca7a480 &g_event_info[0].category 0x10ca7a4a0 &g_event_info[0].flags 0x10ca7a4a4 &g_event_info[0].nparams 0x10ca7a4a8 &g_event_info[0].params 0x10ca7a4ac &g_event_info[0].params[0].name 0x10ca7a4ac &g_event_info[0].params[0].type 0x10ca7a4cc &g_event_info[0].params[0].fmt 0x10ca7a4d0 &g_event_info[0].params[0].info 0x10ca7a4d4 &g_event_info[0].params[0].ninfo 0x10ca7a4dc &g_event_info[0].params[1].name 0x10ca7a4dd &g_event_info[0].params[1].type 0x10ca7a4fd &g_event_info[0].params[1].fmt 0x10ca7a501 &g_event_info[0].params[1].info 0x10ca7a505 &g_event_info[0].params[1].ninfo 0x10ca7a50d

&g_event_info[1].name 0x10ca7aacc &g_event_info[1].category 0x10ca7aaec &g_event_info[1].flags 0x10ca7aaf0 &g_event_info[1].nparams 0x10ca7aaf4 &g_event_info[1].params 0x10ca7aaf8 &g_event_info[1].params[0].name 0x10ca7aaf8 &g_event_info[1].params[0].type 0x10ca7ab18 &g_event_info[1].params[0].fmt 0x10ca7ab1c &g_event_info[1].params[0].info 0x10ca7ab20 &g_event_info[1].params[0].ninfo 0x10ca7ab28 &g_event_info[1].params[1].name 0x10ca7ab29 &g_event_info[1].params[1].type 0x10ca7ab49 &g_event_info[1].params[1].fmt 0x10ca7ab4d &g_event_info[1].params[1].info 0x10ca7ab51 &g_event_info[1].params[1].ninfo 0x10ca7ab59

Since ppm_param_info.ninfo is one byte, most of the ppm_event_info.params elements are misaligned. Either changing ppm_param_info.ninfo to uint32_t or setting __attribute__((packed,aligned(4))) to ppm_param_info fixes the 4 byte alignment issue for me here. That probably won't fix @cho-m's 8 byte alignment issue, unfortunately.

[FedeDP]

/remove-lifecycle stale

[FedeDP]

We need to gather more feedback about how to fix this up... @leogr @Andreagit97 @Molter73 @LucaGuerra

[jasondellaluce]

Unless ppm_param_info and ppm_event_info are used to read trace files, I think the __attribute__((packed,aligned(4))) approach would be the less error-prone here. Is there a way to specify different alignments for different architectures with a macro maybe? (8 for ARM64, 4 for x86_64)

[FedeDP]

ppm_event_info is used by the event_table and describes our events: https://github.com/falcosecurity/libs/blob/master/driver/event_table.c#L12

In an old comment, i noticed that we would lose scap files backward compatibility by touching these structs; but i think i was wrong: the structs only describe our supported events and their supported params, but they say nothing about the expected bytestreams read from scap files; see scap_event_decode_params: https://github.com/falcosecurity/libs/blob/master/userspace/libscap/scap_event.c#L87

[FedeDP]

@geraldcombs shouldn't ppm_param_info be already packed?

struct ppm_param_info {
	char name[PPM_MAX_NAME_LEN];  /**< Parameter name, e.g. 'size'. */
	enum ppm_param_type type; /**< Parameter type, e.g. 'uint16', 'string'... */
	enum ppm_print_format fmt; /**< If this is a numeric parameter, this flag specifies if it should be rendered as decimal or hex. */
	const void *info; /**< If this is a flags parameter, it points to an array of ppm_name_value,
						   if this is a FSRELPATH parameter, it references the related dirfd,
					   else if this is a dynamic parameter it points to an array of ppm_param_info */
	uint8_t ninfo; /**< Number of entry in the info array. */
} _packed;

Btw thanks for fixing the other issue :)

EDIT: btw what about making nparams in ppm_event_info 8B large? ie:

struct ppm_event_info { 
 	char name[PPM_MAX_NAME_LEN]; /**< Name. */ 
 	enum ppm_event_category category; /**< Event category, e.g. 'file', 'net', etc. */ 
 	enum ppm_event_flags flags; /**< flags for this event. */ 
 	uint64_t nparams; /**< Number of parameter in the params array. */ 
 	struct ppm_param_info params[PPM_MAX_EVENT_PARAMS]; /**< parameters descriptions. */ 
 } _packed; 

[geraldcombs]

shouldn't ppm_param_info be already packed?

A single ppm_param_info struct should be naturally aligned on 64-bit systems, but the problem (on my machine at least) is when you have an array of them. Each element in ppm_event_info.params is 49 bytes after the previous element.

what about making nparams in ppm_event_info 8B large?

That would work for ppm_event_info, but we'd have to make sure ppm_param_info is 8B aligned as well, e.g. by making ninfo 8B. I suppose we could also do the following:

• Don't pack on macOS. Maybe add a _packed_if_possible #define, which is empty on macOS and the same as _packed elsewhere?

• As an alternative to resizing structs, fill in the current 8B alignment holes with reserved elements, for example:

struct ppm_param_info {
	char name[PPM_MAX_NAME_LEN];  /**< Parameter name, e.g. 'size'. */
	enum ppm_param_type type; /**< Parameter type, e.g. 'uint16', 'string'... */
	enum ppm_print_format fmt; /**< If this is a numeric parameter, this flag specifies if it should be rendered as decimal or hex. */
	const void *info; /**< If this is a flags parameter, it points to an array of ppm_name_value,
						   if this is a FSRELPATH parameter, it references the related dirfd,
					   else if this is a dynamic parameter it points to an array of ppm_param_info */
	uint8_t ninfo; /**< Number of entry in the info array. */
	uint8_t reserved1[7];
} _packed;

struct ppm_event_info {
	char name[PPM_MAX_NAME_LEN]; /**< Name. */
	enum ppm_event_category category; /**< Event category, e.g. 'file', 'net', etc. */
	enum ppm_event_flags flags; /**< flags for this event. */
	uint32_t nparams; /**< Number of parameter in the params array. */
	uint32_t reserved; // or uint8_t[4], similar to ppm_param_info.
	struct ppm_param_info params[PPM_MAX_EVENT_PARAMS]; /**< parameters descriptions. */
} _packed;

[FedeDP]

Hi @geraldcombs ! In the end, what about completely dropping the packed from these structs? I mean, there is no real gain for packed here, afaik. Do you agree that it would solve the issue? Are you willing to open a PR in that sense? Thank you for your time and investigation!

@terylt @Molter73 do you agree with this solution?

[leogr]

Hi @geraldcombs ! In the end, what about completely dropping the packed from these structs? I mean, there is no real gain for packed here, afaik. Do you agree that it would solve the issue? Are you willing to open a PR in that sense? Thank you for your time and investigation!

Totally agree. It looks like those structs are not serialized, so removing _packed could even improve memory access performances on some archs.

[terylt]

@FedeDP what is the packed used for? As long as it doesn't break reading scap files, I'm fine with it.

[FedeDP]

I think it was just used to save some runtime memory actually, so it should not impact scap files :)