Improve `ASSERT` macro, fix linux file paths in `Taskfile` and hopefully fix the windows release #1295

xTVaser · 2022-04-11T01:37:12Z

For future reference, the regex i used to find the instances - (.*print.*\n){1,5}.*ASSERT$false$

Adds a ASSERT_MSG macro which you can pass a std::string into to be printed, or a string literal. Will also flush stdout and stderr before aborting now -- which should hopefully make logging more consistent.

Hopefully fixed the windows release and task commands on linux should now perform as expected.

… `abort`ing

…ances

common/audio/audio_formats.cpp

water111 · 2022-04-11T01:42:33Z

goalc/regalloc/Allocator.cpp

-          printf("SPILLING FAILED BECAUSE WE COULDN'T FIND A TEMP REGISTER!\n");
-          ASSERT(false);
+          ASSERT_MSG(false,
+                     fmt::format("SPILLING FAILED BECAUSE WE COULDN'T FIND A TEMP REGISTER!"));


you could drop the fmt::format here

water111 · 2022-04-11T01:42:52Z

goalc/emitter/CodeTester.cpp

@@ -133,8 +133,7 @@ void CodeTester::init_code_buffer(int capacity) {
  code_buffer = (u8*)mmap(nullptr, capacity, PROT_EXEC | PROT_READ | PROT_WRITE,
                          MAP_ANONYMOUS | MAP_PRIVATE, 0, 0);
  if (code_buffer == (u8*)(-1)) {
-    printf("[CodeTester] Failed to map memory!\n");
-    ASSERT(false);
+    ASSERT_MSG(false, fmt::format("[CodeTester] Failed to map memory!"));


water111 · 2022-04-11T01:43:08Z

goalc/emitter/CodeTester.h

@@ -91,13 +91,19 @@ class CodeTester {
  /*!
   * Get the name of the given register, for debugging.
   */
-  std::string reg_name(Register x) { return m_info.get_info(x).name; }
+  std::string reg_name(Register x) {
+    return m_info.get_info(x).name;


hm, I think the formatting isn't right here

yeah i have no idea whats going on, my local clang formatter in CLI has formatted things differently than the same command in CI / Visual Studio....

PS C:\Users\xtvas\Repositories\opengoal\jak-project> clang-format -verbose .\goalc\emitter\CodeTester.h ... /*! * Get the name of the given register, for debugging. */ std::string reg_name(Register x) { return m_info.get_info(x).name; }

but in visual studio....

Any ideas on debugging what's going on?

It should find this file:
https://github.com/open-goal/jak-project/blob/master/.clang-format

I think you can specify the file with -style=<path-to-clang-format-file> but it should automatically go up directories until it finds the file.

oh I am wrong. The right thing is:

-style=file

(literally put the word file there)

i think it is, but its hard to tell (i think the BaseOnStyle gets expanded?), what is your output from a clang-format --dump-config?

here's my output. the ColumnLimit: 100 is something that's different from the default config.

--- Language: Cpp AccessModifierOffset: -1 AlignAfterOpenBracket: Align AlignConsecutiveMacros: false AlignConsecutiveAssignments: false AlignConsecutiveDeclarations: false AlignEscapedNewlines: Left AlignOperands: true AlignTrailingComments: true AllowAllArgumentsOnNextLine: true AllowAllConstructorInitializersOnNextLine: true AllowAllParametersOfDeclarationOnNextLine: false AllowShortBlocksOnASingleLine: Never AllowShortCaseLabelsOnASingleLine: false AllowShortFunctionsOnASingleLine: Inline AllowShortLambdasOnASingleLine: All AllowShortIfStatementsOnASingleLine: Never AllowShortLoopsOnASingleLine: false AlwaysBreakAfterDefinitionReturnType: None AlwaysBreakAfterReturnType: None AlwaysBreakBeforeMultilineStrings: true AlwaysBreakTemplateDeclarations: Yes BinPackArguments: true BinPackParameters: false BraceWrapping: AfterCaseLabel: false AfterClass: false AfterControlStatement: false AfterEnum: false AfterFunction: false AfterNamespace: false AfterObjCDeclaration: false AfterStruct: false AfterUnion: false AfterExternBlock: false BeforeCatch: false BeforeElse: false IndentBraces: false SplitEmptyFunction: true SplitEmptyRecord: true SplitEmptyNamespace: true BreakBeforeBinaryOperators: None BreakBeforeBraces: Attach BreakBeforeInheritanceComma: false BreakInheritanceList: BeforeColon BreakBeforeTernaryOperators: true BreakConstructorInitializersBeforeComma: false BreakConstructorInitializers: BeforeColon BreakAfterJavaFieldAnnotations: false BreakStringLiterals: true ColumnLimit: 100 CommentPragmas: '^ IWYU pragma:' CompactNamespaces: false ConstructorInitializerAllOnOneLineOrOnePerLine: true ConstructorInitializerIndentWidth: 4 ContinuationIndentWidth: 4 Cpp11BracedListStyle: true DeriveLineEnding: true DerivePointerAlignment: false DisableFormat: false ExperimentalAutoDetectBinPacking: false FixNamespaceComments: true ForEachMacros: - foreach - Q_FOREACH - BOOST_FOREACH IncludeBlocks: Preserve IncludeCategories: - Regex: '^<ext/.*\.h>' Priority: 2 SortPriority: 0 - Regex: '^<.*\.h>' Priority: 1 SortPriority: 0 - Regex: '^<.*' Priority: 2 SortPriority: 0 - Regex: '.*' Priority: 3 SortPriority: 0 IncludeIsMainRegex: '([-_](test|unittest))?$' IncludeIsMainSourceRegex: '' IndentCaseLabels: true IndentGotoLabels: true IndentPPDirectives: None IndentWidth: 2 IndentWrappedFunctionNames: false JavaScriptQuotes: Leave JavaScriptWrapImports: true KeepEmptyLinesAtTheStartOfBlocks: false MacroBlockBegin: '' MacroBlockEnd: '' MaxEmptyLinesToKeep: 1 NamespaceIndentation: None ObjCBinPackProtocolList: Never ObjCBlockIndentWidth: 2 ObjCSpaceAfterProperty: false ObjCSpaceBeforeProtocolList: true PenaltyBreakAssignment: 2 PenaltyBreakBeforeFirstCallParameter: 1 PenaltyBreakComment: 300 PenaltyBreakFirstLessLess: 120 PenaltyBreakString: 1000 PenaltyBreakTemplateDeclaration: 10 PenaltyExcessCharacter: 1000000 PenaltyReturnTypeOnItsOwnLine: 200 PointerAlignment: Left RawStringFormats: - Language: Cpp Delimiters: - cc - CC - cpp - Cpp - CPP - 'c++' - 'C++' CanonicalDelimiter: '' BasedOnStyle: google - Language: TextProto Delimiters: - pb - PB - proto - PROTO EnclosingFunctions: - EqualsProto - EquivToProto - PARSE_PARTIAL_TEXT_PROTO - PARSE_TEST_PROTO - PARSE_TEXT_PROTO - ParseTextOrDie - ParseTextProtoOrDie CanonicalDelimiter: '' BasedOnStyle: google ReflowComments: true SortIncludes: false SortUsingDeclarations: true SpaceAfterCStyleCast: false SpaceAfterLogicalNot: false SpaceAfterTemplateKeyword: true SpaceBeforeAssignmentOperators: true SpaceBeforeCpp11BracedList: false SpaceBeforeCtorInitializerColon: true SpaceBeforeInheritanceColon: true SpaceBeforeParens: ControlStatements SpaceBeforeRangeBasedForLoopColon: true SpaceInEmptyBlock: false SpaceInEmptyParentheses: false SpacesBeforeTrailingComments: 2 SpacesInAngles: false SpacesInConditionalStatement: false SpacesInContainerLiterals: true SpacesInCStyleCastParentheses: false SpacesInParentheses: false SpacesInSquareBrackets: false SpaceBeforeSquareBrackets: false Standard: Auto StatementMacros: - Q_UNUSED - QT_REQUIRE_VERSION TabWidth: 8 UseCRLF: false UseTab: Never ...

water111 · 2022-04-11T01:46:59Z

common/util/Assert.cpp

+                           int line,
+                           const char* function,
+                           const char* msg) {
+  if ((msg != NULL) && (msg[0] == '\0')) {


I think this condition should be if( !msg || msg[0] == '\0') so we don't try to print a message if its null.

water111 · 2022-04-11T01:47:19Z

common/util/Assert.cpp

+    fprintf(stderr, "Assertion failed: '%s'\n\tMessage: %s\n\tSource: %s:%d\n\tFunction: %s\n",
+            expr, msg, file, line, function);
+  }
+  fflush(stdout);  // ensure any stdout logs are flushed before we terminate


if you're paranoid, you could put fflush(stderr); as well

water111 · 2022-04-11T01:49:16Z

common/util/Assert.h

@@ -3,16 +3,23 @@
 * Custom ASSERT macro
 */

+#include "third-party/fmt/core.h"


can we remove this? I'd rather not include libfmt in more places than we need because it's a huge header-only library that increases compile times. I think we can just put #include <string> instead.

water111 · 2022-04-11T01:51:28Z

common/util/Assert.h

+
+#define ASSERT_MSG(EXPR, STR) \
+  (void)((EXPR) ||            \
+         (private_assert_failed(#EXPR, __FILE__, __LINE__, __PRETTY_FUNCTION__, STR.c_str()), 0))


using .c_str() in the macro like this means it won't work for plain string constants. Maybe we could remove the .c_str() here and have a second private_assert_failed that takes const std::string& msg. This overload of private_assert_failed could then call the const char* msg version.

water111 · 2022-04-11T01:54:14Z

decompiler/data/tpage.cpp

@@ -276,8 +276,7 @@ Texture read_texture(ObjectFileData& data, const std::vector<LinkedWord>& words,

  auto kv = psms.find(tex.psm);
  if (kv == psms.end()) {
-    printf("Got unsupported texture 0x%x!\n", tex.psm);
-    ASSERT(false);
+    ASSERT_MSG(false, fmt::format("Got unsupported texture 0x{0:x}!", tex.psm));


maybe this should be 0x{:x}?

xTVaser added 7 commits April 10, 2022 21:05

ci: fix windows releases (hopefully)

92f375b

scripts: fix Taskfile file references for linux

782f995

asserts: add ASSERT_MSG macro and ensure stdout is flushed before…

4fd1db2

… `abort`ing

asserts: refactor all assert(false); with a preceeding message inst…

d6847e0

…ances

lint: format

6d4d6da

temp...

a668e7b

fix compiler errors

2c5f32e

water111 reviewed Apr 11, 2022

View reviewed changes

xTVaser added 3 commits April 10, 2022 22:24

assert: allow for string literals in ASSERT_MSG

b230749

lint: formatting

3643807

revert temp change for testing

1023c1f

water111 merged commit c4a9257 into open-goal:master Apr 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve `ASSERT` macro, fix linux file paths in `Taskfile` and hopefully fix the windows release #1295

Improve `ASSERT` macro, fix linux file paths in `Taskfile` and hopefully fix the windows release #1295

xTVaser commented Apr 11, 2022 •

edited

Loading

water111 Apr 11, 2022

water111 Apr 11, 2022

water111 Apr 11, 2022

xTVaser Apr 11, 2022

xTVaser Apr 11, 2022

water111 Apr 11, 2022

water111 Apr 11, 2022

xTVaser Apr 11, 2022 •

edited

Loading

water111 Apr 11, 2022

water111 Apr 11, 2022

water111 Apr 11, 2022

water111 Apr 11, 2022

water111 Apr 11, 2022

water111 Apr 11, 2022

Improve ASSERT macro, fix linux file paths in Taskfile and hopefully fix the windows release #1295

Improve ASSERT macro, fix linux file paths in Taskfile and hopefully fix the windows release #1295

Conversation

xTVaser commented Apr 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xTVaser Apr 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Improve `ASSERT` macro, fix linux file paths in `Taskfile` and hopefully fix the windows release #1295

Improve `ASSERT` macro, fix linux file paths in `Taskfile` and hopefully fix the windows release #1295

xTVaser commented Apr 11, 2022 •

edited

Loading

xTVaser Apr 11, 2022 •

edited

Loading