Skip to content

Conversation

@phuang
Copy link
Contributor

@phuang phuang commented Dec 1, 2024

OHOS uses musl libc which doesn't have get_current_dir_name(). Removing get_current_dir_name related dfsw and dfso methods to fix the problem.

@github-actions
Copy link

github-actions bot commented Dec 1, 2024

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@llvmbot
Copy link
Member

llvmbot commented Dec 1, 2024

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: Peng Huang (phuang)

Changes

OHOS uses musl libc which doesn't have get_current_dir_name(). Removing get_current_dir_name related dfsw and dfso methods to fix the problem.


Full diff: https://github.com/llvm/llvm-project/pull/118193.diff

1 Files Affected:

  • (modified) compiler-rt/lib/dfsan/dfsan_custom.cpp (+3)
diff --git a/compiler-rt/lib/dfsan/dfsan_custom.cpp b/compiler-rt/lib/dfsan/dfsan_custom.cpp
index dbc00d7ac3ea39..29800479d20e66 100644
--- a/compiler-rt/lib/dfsan/dfsan_custom.cpp
+++ b/compiler-rt/lib/dfsan/dfsan_custom.cpp
@@ -1077,6 +1077,8 @@ char *__dfso_getcwd(char *buf, size_t size, dfsan_label buf_label,
   return ret;
 }
 
+#if !defined(__OHOS__)
+// OpenHOS uses musl libc which doesn't have get_current_dir_name()
 SANITIZER_INTERFACE_ATTRIBUTE
 char *__dfsw_get_current_dir_name(dfsan_label *ret_label) {
   char *ret = get_current_dir_name();
@@ -1091,6 +1093,7 @@ char *__dfso_get_current_dir_name(dfsan_label *ret_label,
                                   dfsan_origin *ret_origin) {
   return __dfsw_get_current_dir_name(ret_label);
 }
+#endif
 
 // This function is only available for glibc 2.25 or newer.  Mark it weak so
 // linking succeeds with older glibcs.

@thurstond
Copy link
Contributor

Thanks for the patch! Sanitizers have a #if SANITIZER_MUSL macro. Could you please try whether that works instead? It would fix the musl problem more generally than !defined(__OHOS__).

Adding @browneee (dfsan maintainer) since there are no reviewers added.

@thurstond thurstond requested a review from browneee December 1, 2024 04:18
@phuang
Copy link
Contributor Author

phuang commented Dec 1, 2024

Thanks for the patch! Sanitizers have a #if SANITIZER_MUSL macro. Could you please try whether that works instead? It would fix the musl problem more generally than !defined(__OHOS__).

Adding @browneee (dfsan maintainer) since there are no reviewers added.

Done.

@browneee
Copy link
Contributor

browneee commented Dec 1, 2024

Thanks @thurstond!

MUSL appears to have this, but only under _GNU_SOURCE:

glibc also only has this under _GNU_SOURCE (via glibc's internal set __USE_GNU).

Does your system have _GNU_SOURCE defined?

I think it would be better to put this function under #ifdef _GNU_SOURCE.


Also note SANITIZER_MUSL looks at __GLIBC__, not _GNU_SOURCE so I think it would be less precise than using _GNU_SOURCE here.

#if defined(__GLIBC__)
# define SANITIZER_GLIBC 1

#if SANITIZER_LINUX && !SANITIZER_GLIBC && !SANITIZER_ANDROID
# define SANITIZER_MUSL 1

@phuang
Copy link
Contributor Author

phuang commented Dec 1, 2024

Thanks for the review.

I checked ohos's musl libc header. It doesn't have get_current_dir_name(), however it does check _GNU_SOURCE. So checking _GNU_SOURCE may not work.

https://gitee.com/openharmony/third_party_musl/blob/master/include/unistd.h#L194

@browneee
Copy link
Contributor

browneee commented Dec 1, 2024

Your header does appear to have get_current_dir_name under _GNU_SOURCE:
https://gitee.com/openharmony/third_party_musl/blob/master/include/unistd.h#L251

@phuang
Copy link
Contributor Author

phuang commented Dec 2, 2024

You are right. But the openharmony os sdk's header file doesn't have that function. It is so weird. I will ask openharmony community about the problem.

@phuang
Copy link
Contributor Author

phuang commented Jan 1, 2025

Hi @browneee , Unfortunately, the ohos sdk's header file doesn't have get_current_dir_name() function. Even if sdk is updated to include get_current_dir_name(), I think llvm still need to compiled with older ohos sdk. So I think it is better to guard __dfsw_get_current_dir_name() and __dfso_get_current_dir_name() with #ifndef __OHOS__ in llvm source code. I updated this PR, please take a look. thanks

OpenHarmony's libc doesn't have get_current_dir_name(), So guard
get_current_dir_name() related dfsw and dfso methods with #ifndef
__OHOS__.
@phuang phuang force-pushed the wip_compile_error_on_ohos branch from 4caf0f7 to 867e458 Compare January 2, 2025 01:52
@browneee
Copy link
Contributor

browneee commented Jan 2, 2025

Imo the correct solution here is fix openharmony os's sdk by adding the missing function.

This approach would be better for several reasons:

  • If you don't fix OHOS musl installation, you'll eventually get similar issues when building other software that uses this function. Imo you'll probably save OHOS developes/users time by fixing the root cause.
  • If you do this #ifdef, then later fix the OHOS sdk (add the missing function), people may hit issues where they have a version of llvm sources which #ifdef the function out, even though it is now present in OHOS.
  • This #ifdef is polluting the sanitizer code with a tiny bit of extra complexity for a bug (afaict) in a relatively uncommon platform.
  • It seems fair for you to say that you only support DFSan on OHOS for versions of OHOS after SDK is fixed (DFSan being more of a an optional-extra feature in LLVM, rather than something essential for compilation).

Was there any explanation from the openharmony community why this function was missing?

Between fixing openharmony sdk vs your latest patch, do you still feel your patch is the right solution?

@phuang
Copy link
Contributor Author

phuang commented Jan 2, 2025

Was there any explanation from the openharmony community why this function was missing?

They told me this function is not exposed in ohos sdk, but they didn't explain why. I will ask them to explain it.

Between fixing openharmony sdk vs your latest patch, do you still feel your patch is the right solution?

Another solution, I can test get_current_dir_name() in cmake script, and guard it with HAS_GET_CURRENT_DIR_NAME macro. what do you think?

@browneee
Copy link
Contributor

browneee commented Jan 2, 2025

Another solution, I can test get_current_dir_name() in cmake script, and guard it with HAS_GET_CURRENT_DIR_NAME macro. what do you think?

This would address this issue:

  • If you do this #ifdef, then later fix the OHOS sdk (add the missing function), people may hit issues where they have a version of llvm sources which #ifdef the function out, even though it is now present in OHOS.

But it would slightly increase this issue:

This #ifdef is polluting the sanitizer code with a tiny bit of extra complexity for a bug (afaict) in a relatively uncommon platform.

I would still recommend fixing openharmony os's sdk by adding the missing function.

They told me this function is not exposed in ohos sdk, but they didn't explain why. I will ask them to explain it.

Thanks! It would be good to at least hear why, before proceeding with workarounds.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants