Skip to content

ohos: Use custom domain when logging with hilog #583

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jschwe
Copy link
Member

@jschwe jschwe commented May 21, 2025

This modifies the existing patch to redirect log messages to hilog to use a non-zero domain, which allows us to filter logs via the domain on ohos devices.
We set 0xE0C3 for Rust code in servo, using a different number allows us to seperate spidermonkey from servo logs if wanted.
This also uncovered and fixed two seperate issues with the patch:

  • We need to use the XP_OHOS define, since OHOS is never defined by us
  • The target_is_ohos check returning False still causes set_define to set the define (differing from the documentation), so we need to return None.

Fixes #582

@jschwe jschwe force-pushed the jschwender/hilog_domain branch from 760ddba to cdb1229 Compare May 21, 2025 02:23
@jschwe jschwe marked this pull request as ready for review May 21, 2025 03:15
@@ -47,7 +47,7 @@ index 3cfc92533..9c487ac45 100644
+#ifdef ANDROID
+ __android_log_print(ANDROID_LOG_ERROR, "Gecko", "mozalloc_abort: %s", msg);
+#elif defined(OHOS)
+ (void) OH_LOG_Print(LOG_APP, LOG_ERROR, 0, "Gecko",
+ (void) OH_LOG_Print(LOG_APP, LOG_ERROR, 0xEOC4, "Gecko",
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to define a constant for this domain value, maybe in the init.configure, rather than hard coding it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that is definitely better than copying the same value everywhere.

@jschwe jschwe force-pushed the jschwender/hilog_domain branch from cdb1229 to 8af55cb Compare May 21, 2025 06:58
+def ohos_hilog_domain(target):
+ return "0xE0C4"
+
+set_define("OHOS_LOG_DOMAIN", ohos_hilog_domain)
Copy link
Member

Choose a reason for hiding this comment

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

It looks like set_define also accepts a when argument. Is it not possible to use it here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I checked the implementation (set_define_impl) and the doc string says that when the value is false, the define will be explicitly undefined (-U).

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, did you want to suggest to just remove the ohos_hilog_domain function and instead use when? that makes sense to me.

Copy link
Member

Choose a reason for hiding this comment

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

Right, my idea was to see if we can just do set_define("OHOS_LOG_DOMAIN", "0xE0C4", when=target_is_ohos)

This modifies the existing patch to redirect log messages to
hilog to use a non-zero domain, which allows us to filter
logs via the domain on ohos devices.
We set 0xE0C3 for Rust code in servo, using a different number
allows us to seperate spidermonkey from servo logs if wanted.

Signed-off-by: Jonathan Schwender <[email protected]>
@jschwe jschwe force-pushed the jschwender/hilog_domain branch from 8af55cb to 882d13a Compare May 21, 2025 09:34
@jschwe
Copy link
Member Author

jschwe commented May 22, 2025

I'm not really sure what is going on here:

@depends(target)
def target_is_ohos(target):
    return False


set_define("XP_OHOS", target_is_ohos)

This causes XP_OHOS to be defined.

@depends(target)
def target_is_ohos(target):
    return None


set_define("XP_OHOS", target_is_ohos)

This does not define XP_OHOS. This seems really weird to me, and goes against the documentation of set_define_impl and also how other defines are set.

Signed-off-by: Jonathan Schwender <[email protected]>
@jschwe jschwe force-pushed the jschwender/hilog_domain branch from c91b2b9 to 04b67af Compare May 22, 2025 06:50
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.

ohos: Set log domain
2 participants