Skip to content

Conversation

@AngheloAlf
Copy link
Collaborator

This fixes building on macos due to a kinda specific issue with Apple clang.

The issue

When trying to build on Macos (specifically MacOS 12, Monteray with Apple clang 13.0.0 (clang-1300.0.29.30), idk if other versions have this issue too) make stops with the following error from atblgen:

Failed to match line 1: ""
regexec error: "regexec() failed to match"
Error: Malformed build/n64-us/assets/audio/sequence_order.in?

atblgen makes the assumption that the sequence_order.in file has no extra data, spaces, empty lines, etc. but the file somehow ends up having empty lines between each line on macos.

This file is created by using the C preprocessor to process include/tables/sequence_table.h.
In normal circumstances this file should look like this snip,

(Sequence_0,NA_BGM_GENERAL_SFX)
(Sequence_1,NA_BGM_AMBIENCE)
(Sequence_2,NA_BGM_TERMINA_FIELD)
(Sequence_3,NA_BGM_CHASE)
(Sequence_4,NA_BGM_MAJORAS_THEME)

but it ends up looking like this instead

(Sequence_0,NA_BGM_GENERAL_SFX)

(Sequence_1,NA_BGM_AMBIENCE)

(Sequence_2,NA_BGM_TERMINA_FIELD)

(Sequence_3,NA_BGM_CHASE)

(Sequence_4,NA_BGM_MAJORAS_THEME)

which atblgen doesn't like.

I believe this happens because there are lines with comments between each macro in sequence_table.h and for some reason this Apple clang version decided to preserve those empty lines

The fix

The fix just makes atblgen skip empty lines.

Misc

I threw atblgen to valgrind to check the fix was working as intended and noted a bunch of memory leaks, so I fixed them.

I also noted the tools/audio makefile was not using OPTFLAGS when building those tools, so I fixed that too.

// We expect one entry per line, gather the total length
size_t total_size = 0;
for (char *p = filedata; *p != '\0'; p++) {
for (char *p = filedata; *p != '\0';) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mind adding skipping initial whitespace up to the first non-empty line as well? The behavior right now would miss initial whitespace but handle all other cases properly, might as well cover all cases of extra empty lines?

Comment on lines +587 to +593
free(final_seqdata);
for (int i = 0; i < num_sequence_files; i++) {
free(file_data[i].name);
free(file_data[i].elf_path);
}
free(file_data);
free_seq_order(&order);
Copy link
Contributor

Choose a reason for hiding this comment

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

On this, I wouldn't strictly call this a memory leak, the program is about to exit. I didn't bother cleaning up the memory for that reason, but since you've added it now it's not a big deal to keep it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants