| Message ID | 875y6qeqma.fsf@oldenburg.str.redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | aarch64: Fix warnings during libgcc build | expand |
On 11/07/2023 10:37, Florian Weimer via Gcc-patches wrote: > libgcc/ > > * config/aarch64/aarch64-unwind.h (aarch64_cie_signed_with_b_key): > Add missing const qualifier. Cast from const unsigned char * > to const char *. Use __builtin_strchr to avoid an implicit > function declaration. > * config/aarch64/linux-unwind.h (aarch64_fallback_frame_state): > Add missing cast. > > --- > diff --git a/libgcc/config/aarch64/linux-unwind.h b/libgcc/config/aarch64/linux-unwind.h > index 00eba866049..93da7a9537d 100644 > --- a/libgcc/config/aarch64/linux-unwind.h > +++ b/libgcc/config/aarch64/linux-unwind.h > @@ -77,7 +77,7 @@ aarch64_fallback_frame_state (struct _Unwind_Context *context, > } > > rt_ = context->cfa; > - sc = &rt_->uc.uc_mcontext; > + sc = (struct sigcontext *) &rt_->uc.uc_mcontext; > > /* This define duplicates the definition in aarch64.md */ > #define SP_REGNUM 31 > > This looks somewhat dubious. I'm not particularly familiar with the kernel headers, but a quick look suggests an mcontext_t is nothing like a sigcontext_t. So isn't the cast just papering over some more fundamental problem? R.
On 11/07/2023 15:54, Richard Earnshaw (lists) via Gcc-patches wrote: > On 11/07/2023 10:37, Florian Weimer via Gcc-patches wrote: >> libgcc/ >> >> * config/aarch64/aarch64-unwind.h (aarch64_cie_signed_with_b_key): >> Add missing const qualifier. Cast from const unsigned char * >> to const char *. Use __builtin_strchr to avoid an implicit >> function declaration. >> * config/aarch64/linux-unwind.h (aarch64_fallback_frame_state): >> Add missing cast. >> >> --- >> diff --git a/libgcc/config/aarch64/linux-unwind.h >> b/libgcc/config/aarch64/linux-unwind.h >> index 00eba866049..93da7a9537d 100644 >> --- a/libgcc/config/aarch64/linux-unwind.h >> +++ b/libgcc/config/aarch64/linux-unwind.h >> @@ -77,7 +77,7 @@ aarch64_fallback_frame_state (struct _Unwind_Context >> *context, >> } >> rt_ = context->cfa; >> - sc = &rt_->uc.uc_mcontext; >> + sc = (struct sigcontext *) &rt_->uc.uc_mcontext; >> /* This define duplicates the definition in aarch64.md */ >> #define SP_REGNUM 31 >> >> > > This looks somewhat dubious. I'm not particularly familiar with the > kernel headers, but a quick look suggests an mcontext_t is nothing like > a sigcontext_t. So isn't the cast just papering over some more > fundamental problem? > > R. Sorry, I was looking at the wrong set of headers. It looks like these have to match. But in that case, I think we should have a comment about that here to explain the suspicious cast. R.
* Richard Earnshaw: > On 11/07/2023 10:37, Florian Weimer via Gcc-patches wrote: >> libgcc/ >> * config/aarch64/aarch64-unwind.h >> (aarch64_cie_signed_with_b_key): >> Add missing const qualifier. Cast from const unsigned char * >> to const char *. Use __builtin_strchr to avoid an implicit >> function declaration. >> * config/aarch64/linux-unwind.h (aarch64_fallback_frame_state): >> Add missing cast. >> --- >> diff --git a/libgcc/config/aarch64/linux-unwind.h b/libgcc/config/aarch64/linux-unwind.h >> index 00eba866049..93da7a9537d 100644 >> --- a/libgcc/config/aarch64/linux-unwind.h >> +++ b/libgcc/config/aarch64/linux-unwind.h >> @@ -77,7 +77,7 @@ aarch64_fallback_frame_state (struct _Unwind_Context *context, >> } >> rt_ = context->cfa; >> - sc = &rt_->uc.uc_mcontext; >> + sc = (struct sigcontext *) &rt_->uc.uc_mcontext; >> /* This define duplicates the definition in aarch64.md */ >> #define SP_REGNUM 31 >> > > This looks somewhat dubious. I'm not particularly familiar with the > kernel headers, but a quick look suggests an mcontext_t is nothing > like a sigcontext_t. So isn't the cast just papering over some more > fundamental problem? I agree it looks dubious. Note that it's struct sigcontext, not (not-struct) sigcontext_t. I don't know why the uc_mcontext members aren't accessed directly, so I can't really write a good comment about it. Obviously it works quite well as-is. 8-) Similar code is present in many, many Linux targets. Thanks, Florian
The 07/11/2023 17:20, Florian Weimer wrote: > * Richard Earnshaw: > > > On 11/07/2023 10:37, Florian Weimer via Gcc-patches wrote: > >> libgcc/ > >> * config/aarch64/aarch64-unwind.h > >> (aarch64_cie_signed_with_b_key): > >> Add missing const qualifier. Cast from const unsigned char * > >> to const char *. Use __builtin_strchr to avoid an implicit > >> function declaration. > >> * config/aarch64/linux-unwind.h (aarch64_fallback_frame_state): > >> Add missing cast. > >> --- > >> diff --git a/libgcc/config/aarch64/linux-unwind.h b/libgcc/config/aarch64/linux-unwind.h > >> index 00eba866049..93da7a9537d 100644 > >> --- a/libgcc/config/aarch64/linux-unwind.h > >> +++ b/libgcc/config/aarch64/linux-unwind.h > >> @@ -77,7 +77,7 @@ aarch64_fallback_frame_state (struct _Unwind_Context *context, > >> } > >> rt_ = context->cfa; > >> - sc = &rt_->uc.uc_mcontext; > >> + sc = (struct sigcontext *) &rt_->uc.uc_mcontext; > >> /* This define duplicates the definition in aarch64.md */ > >> #define SP_REGNUM 31 > >> > > > > This looks somewhat dubious. I'm not particularly familiar with the > > kernel headers, but a quick look suggests an mcontext_t is nothing > > like a sigcontext_t. So isn't the cast just papering over some more > > fundamental problem? > > I agree it looks dubious. Note that it's struct sigcontext, not > (not-struct) sigcontext_t. I don't know why the uc_mcontext members > aren't accessed directly, so I can't really write a good comment about > it. historically glibc typedefed mcontext_t to linux struct sigcontext so this used to work fine. (i dont know about other os-es) then at some point glibc fixed the namespace polluting fields when building for posix which required a separate mcontext_t. i guess either fix works: moving to the correct mcontext_t or to cast to struct sigcontext, but the former means the fields must be changed when building in a posix conforming mode (i guess libgcc is built with _GNU_SOURCE so may not be an issue) and they may be different across different libcs (or even different versions of glibc) then. > > Obviously it works quite well as-is. 8-) Similar code is present in > many, many Linux targets. > > Thanks, > Florian >
diff --git a/libgcc/config/aarch64/aarch64-unwind.h b/libgcc/config/aarch64/aarch64-unwind.h index 3ad2f8239ed..30e428862c4 100644 --- a/libgcc/config/aarch64/aarch64-unwind.h +++ b/libgcc/config/aarch64/aarch64-unwind.h @@ -40,8 +40,8 @@ aarch64_cie_signed_with_b_key (struct _Unwind_Context *context) const struct dwarf_cie *cie = get_cie (fde); if (cie != NULL) { - char *aug_str = cie->augmentation; - return strchr (aug_str, 'B') == NULL ? 0 : 1; + const char *aug_str = (const char *) cie->augmentation; + return __builtin_strchr (aug_str, 'B') == NULL ? 0 : 1; } } return 0; diff --git a/libgcc/config/aarch64/linux-unwind.h b/libgcc/config/aarch64/linux-unwind.h index 00eba866049..93da7a9537d 100644 --- a/libgcc/config/aarch64/linux-unwind.h +++ b/libgcc/config/aarch64/linux-unwind.h @@ -77,7 +77,7 @@ aarch64_fallback_frame_state (struct _Unwind_Context *context, } rt_ = context->cfa; - sc = &rt_->uc.uc_mcontext; + sc = (struct sigcontext *) &rt_->uc.uc_mcontext; /* This define duplicates the definition in aarch64.md */ #define SP_REGNUM 31