Two questions about address_space attribute

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

Two questions about address_space attribute

Kristof Beyls via cfe-dev
Hi,

Currently, I am thinking to add address_space attribute support for
x86_64 and BPF to compile linux kernel. The main goal is to get
address_space information into debug_info and then later to dwarf/BTF.
But address_space enforcement itself can also help with better code
for the kernel.

The RFC patch is here:
    https://reviews.llvm.org/D69393

During experiments, we found two issues which I would like to get
expert opinion:

issue 1: address_space attributes on function pointers
=========================================

clang does not allow address_space attribute for function pointers,
specifically, in clang/lib/Sema/SemaType.cpp,

// ISO/IEC TR 18037 S5.3 (amending C99 6.7.3): "A function type shall not be
// qualified by an address-space qualifier."
if (Type->isFunctionType()) {
  S.Diag(Attr.getLoc(), diag::err_attribute_address_function_type);
  Attr.setInvalid();
  return;
}

But linux kernel tries to annotate signal handling function pointer
with address space to indicate it is accessing user space.

typedef __signalfn_t __user *__sighandler_t;
typedef __restorefn_t __user *__sigrestore_t;

Such attribute makes sense for linux since indeed the signal handler
code resides in user space and the kernel pointer
merely points to user memory here.

What is the rationale for this restriction in standard? How we could
make clang tolerate such restrictions for X86/BPF/...?

issue 2: typeof(*ptr) where ptr has address_space attribute.

-bash-4.4$ cat t2.c
int test(int __attribute__((address_space(1))) *p) {
#ifdef NO_TYPEOF
  int t = *p;
#else
  typeof(*p) t = *p;
#endif
  return t;
}
-bash-4.4$ clang -c t2.c
t2.c:5:14: error: automatic variable qualified with an address space
  typeof(*p) t = *p;
             ^
1 error generated.
-bash-4.4$ clang -DNO_TYPEOF -c t2.c
-bash-4.4$

The IR generated without NO_TYPEOF macro:
  %p.addr = alloca i32 addrspace(1)*, align 8
  %t = alloca i32, align 4
  store i32 addrspace(1)* %p, i32 addrspace(1)** %p.addr, align 8
  %0 = load i32 addrspace(1)*, i32 addrspace(1)** %p.addr, align 8
  %1 = load i32, i32 addrspace(1)* %0, align 4
  store i32 %1, i32* %t, align 4
  %2 = load i32, i32* %t, align 4
  ret i32 %2

So we can directly load a i32 addrspacee(1)* to an i32. Maybe
typeof(*p) should just i32 and we should not emit the error at all?

Thanks,

Yonghong
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Two questions about address_space attribute

Kristof Beyls via cfe-dev
Hi, Richard Smith and Yaxun Liu,

You probably have expertise on the two questions below related
to address space. Could you comment on my questions below?

Thanks!

Yonghong

On Thu, Nov 7, 2019 at 10:29 AM Y Song <[hidden email]> wrote:

>
> Hi,
>
> Currently, I am thinking to add address_space attribute support for
> x86_64 and BPF to compile linux kernel. The main goal is to get
> address_space information into debug_info and then later to dwarf/BTF.
> But address_space enforcement itself can also help with better code
> for the kernel.
>
> The RFC patch is here:
>     https://reviews.llvm.org/D69393
>
> During experiments, we found two issues which I would like to get
> expert opinion:
>
> issue 1: address_space attributes on function pointers
> =========================================
>
> clang does not allow address_space attribute for function pointers,
> specifically, in clang/lib/Sema/SemaType.cpp,
>
> // ISO/IEC TR 18037 S5.3 (amending C99 6.7.3): "A function type shall not be
> // qualified by an address-space qualifier."
> if (Type->isFunctionType()) {
>   S.Diag(Attr.getLoc(), diag::err_attribute_address_function_type);
>   Attr.setInvalid();
>   return;
> }
>
> But linux kernel tries to annotate signal handling function pointer
> with address space to indicate it is accessing user space.
>
> typedef __signalfn_t __user *__sighandler_t;
> typedef __restorefn_t __user *__sigrestore_t;
>
> Such attribute makes sense for linux since indeed the signal handler
> code resides in user space and the kernel pointer
> merely points to user memory here.
>
> What is the rationale for this restriction in standard? How we could
> make clang tolerate such restrictions for X86/BPF/...?
>
> issue 2: typeof(*ptr) where ptr has address_space attribute.
>
> -bash-4.4$ cat t2.c
> int test(int __attribute__((address_space(1))) *p) {
> #ifdef NO_TYPEOF
>   int t = *p;
> #else
>   typeof(*p) t = *p;
> #endif
>   return t;
> }
> -bash-4.4$ clang -c t2.c
> t2.c:5:14: error: automatic variable qualified with an address space
>   typeof(*p) t = *p;
>              ^
> 1 error generated.
> -bash-4.4$ clang -DNO_TYPEOF -c t2.c
> -bash-4.4$
>
> The IR generated without NO_TYPEOF macro:
>   %p.addr = alloca i32 addrspace(1)*, align 8
>   %t = alloca i32, align 4
>   store i32 addrspace(1)* %p, i32 addrspace(1)** %p.addr, align 8
>   %0 = load i32 addrspace(1)*, i32 addrspace(1)** %p.addr, align 8
>   %1 = load i32, i32 addrspace(1)* %0, align 4
>   store i32 %1, i32* %t, align 4
>   %2 = load i32, i32* %t, align 4
>   ret i32 %2
>
> So we can directly load a i32 addrspacee(1)* to an i32. Maybe
> typeof(*p) should just i32 and we should not emit the error at all?
>
> Thanks,
>
> Yonghong
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Two questions about address_space attribute

Kristof Beyls via cfe-dev
+ John Matt

My comments are below.

Sam

-----Original Message-----
From: Y Song <[hidden email]>
Sent: Tuesday, November 12, 2019 10:15 AM
To: [hidden email]
Cc: Andrii Nakryiko <[hidden email]>; Alexei Starovoitov <[hidden email]>; [hidden email]; Liu, Yaxun (Sam) <[hidden email]>
Subject: Re: Two questions about address_space attribute

[CAUTION: External Email]

Hi, Richard Smith and Yaxun Liu,

You probably have expertise on the two questions below related to address space. Could you comment on my questions below?

Thanks!

Yonghong

On Thu, Nov 7, 2019 at 10:29 AM Y Song <[hidden email]> wrote:

>
> Hi,
>
> Currently, I am thinking to add address_space attribute support for
> x86_64 and BPF to compile linux kernel. The main goal is to get
> address_space information into debug_info and then later to dwarf/BTF.
> But address_space enforcement itself can also help with better code
> for the kernel.
>
> The RFC patch is here:
>     https://reviews.llvm.org/D69393
>
> During experiments, we found two issues which I would like to get
> expert opinion:
>
> issue 1: address_space attributes on function pointers
> =========================================
>
> clang does not allow address_space attribute for function pointers,
> specifically, in clang/lib/Sema/SemaType.cpp,
>
> // ISO/IEC TR 18037 S5.3 (amending C99 6.7.3): "A function type shall
> not be // qualified by an address-space qualifier."
> if (Type->isFunctionType()) {
>   S.Diag(Attr.getLoc(), diag::err_attribute_address_function_type);
>   Attr.setInvalid();
>   return;
> }
>
> But linux kernel tries to annotate signal handling function pointer
> with address space to indicate it is accessing user space.
>
> typedef __signalfn_t __user *__sighandler_t; typedef __restorefn_t
> __user *__sigrestore_t;
>
> Such attribute makes sense for linux since indeed the signal handler
> code resides in user space and the kernel pointer merely points to
> user memory here.
>
> What is the rationale for this restriction in standard? How we could
> make clang tolerate such restrictions for X86/BPF/...?
>

I think we may allow function to be in a non-default address space. The issue is that there may be lots of places in clang or llvm assuming function in default address space, which needs to be fixed.

> issue 2: typeof(*ptr) where ptr has address_space attribute.
>
> -bash-4.4$ cat t2.c
> int test(int __attribute__((address_space(1))) *p) { #ifdef NO_TYPEOF
>   int t = *p;
> #else
>   typeof(*p) t = *p;
> #endif
>   return t;
> }
> -bash-4.4$ clang -c t2.c
> t2.c:5:14: error: automatic variable qualified with an address space
>   typeof(*p) t = *p;
>              ^
> 1 error generated.

Llvm assumes stack address space (alloca address space) is determined by data layout. By default it is 0 but it can be changed by specify a different value in data layout. E.g. amdgcn has alloca address space to be 5. It cannot be explicitly specified in source.

> -bash-4.4$ clang -DNO_TYPEOF -c t2.c
> -bash-4.4$
>
> The IR generated without NO_TYPEOF macro:
>   %p.addr = alloca i32 addrspace(1)*, align 8
>   %t = alloca i32, align 4
>   store i32 addrspace(1)* %p, i32 addrspace(1)** %p.addr, align 8
>   %0 = load i32 addrspace(1)*, i32 addrspace(1)** %p.addr, align 8
>   %1 = load i32, i32 addrspace(1)* %0, align 4
>   store i32 %1, i32* %t, align 4
>   %2 = load i32, i32* %t, align 4
>   ret i32 %2
>
> So we can directly load a i32 addrspacee(1)* to an i32. Maybe
> typeof(*p) should just i32 and we should not emit the error at all?
>
> Thanks,
>
> Yonghong
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Two questions about address_space attribute

Kristof Beyls via cfe-dev

On 12 Nov 2019, at 11:16, Liu, Yaxun (Sam) via cfe-dev wrote:

+ John Matt

My comments are below.

Sam

-----Original Message-----
From: Y Song <[hidden email]>
Sent: Tuesday, November 12, 2019 10:15 AM
To: [hidden email]
Cc: Andrii Nakryiko <[hidden email]>; Alexei Starovoitov <[hidden email]>; [hidden email]; Liu, Yaxun (Sam) <[hidden email]>
Subject: Re: Two questions about address_space attribute

[CAUTION: External Email]

Hi, Richard Smith and Yaxun Liu,

You probably have expertise on the two questions below related to address space. Could you comment on my questions below?

Thanks!

Yonghong

On Thu, Nov 7, 2019 at 10:29 AM Y Song <[hidden email]> wrote:

Hi,

Currently, I am thinking to add address_space attribute support for
x86_64 and BPF to compile linux kernel. The main goal is to get
address_space information into debug_info and then later to dwarf/BTF.
But address_space enforcement itself can also help with better code
for the kernel.

The RFC patch is here:
https://reviews.llvm.org/D69393

During experiments, we found two issues which I would like to get
expert opinion:

issue 1: address_space attributes on function pointers
=========================================

clang does not allow address_space attribute for function pointers,
specifically, in clang/lib/Sema/SemaType.cpp,

// ISO/IEC TR 18037 S5.3 (amending C99 6.7.3): "A function type shall
not be // qualified by an address-space qualifier."
if (Type->isFunctionType()) {
S.Diag(Attr.getLoc(), diag::err_attribute_address_function_type);
Attr.setInvalid();
return;
}

But linux kernel tries to annotate signal handling function pointer
with address space to indicate it is accessing user space.

typedef __signalfn_t __user *__sighandler_t; typedef __restorefn_t
__user *__sigrestore_t;

Such attribute makes sense for linux since indeed the signal handler
code resides in user space and the kernel pointer merely points to
user memory here.

What is the rationale for this restriction in standard? How we could
make clang tolerate such restrictions for X86/BPF/...?

I can’t actually speak for the C committee, but I’ll try to answer anyway. As I understand it, there’s two main things.

The first is that data pointers may already be in a different address space from function pointers. Standard C does not actually allow function pointers to be converted to data pointers and vice-versa; that’s an extension, albeit a basically universal one and indeed one mandated by POSIX because of dlsym. Harvard architectures put code in a separate address space, which is often significantly larger than the address space for data, and C supports that. So while it’s not abstractly illogical for the code space to be split into address spaces, the idea of applying the same address spaces to both data and function pointers isn’t totally aligned with C.

The second is just that the traditional hardware-supported uses of address spaces are centered around data, not code. A processor might have different instructions to load/store from different address spaces, and handling those different address spaces correctly and efficiently is important to expose in a systems programming language. While processors often also have different instructions for e.g. near/far calls, this is rarely useful to expose in the programming language, since it usually only comes up in e.g. the userspace/kernelspace boundary, which is so important and fiddly that’s it’s usually written in assembly anyway.

I think we may allow function to be in a non-default address space. The issue is that there may be lots of places in clang or llvm assuming function in default address space, which needs to be fixed.

Yes, I think LLVM might currently assume a single address space for all code. I would want to see signs of progress on that front before agreeing to an extension of address spaces to function pointer types.

John.


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Two questions about address_space attribute

Kristof Beyls via cfe-dev
In reply to this post by Kristof Beyls via cfe-dev
> >
> > Currently, I am thinking to add address_space attribute support for
> > x86_64 and BPF to compile linux kernel. The main goal is to get
> > address_space information into debug_info and then later to dwarf/BTF.
> > But address_space enforcement itself can also help with better code
> > for the kernel.
> >
> > The RFC patch is here:
> >     https://reviews.llvm.org/D69393
> >
> > During experiments, we found two issues which I would like to get
> > expert opinion:
> >
> > issue 1: address_space attributes on function pointers
> > =========================================
> >
> > clang does not allow address_space attribute for function pointers,
> > specifically, in clang/lib/Sema/SemaType.cpp,
> >
> > // ISO/IEC TR 18037 S5.3 (amending C99 6.7.3): "A function type shall
> > not be // qualified by an address-space qualifier."
> > if (Type->isFunctionType()) {
> >   S.Diag(Attr.getLoc(), diag::err_attribute_address_function_type);
> >   Attr.setInvalid();
> >   return;
> > }
> >
> > But linux kernel tries to annotate signal handling function pointer
> > with address space to indicate it is accessing user space.
> >
> > typedef __signalfn_t __user *__sighandler_t; typedef __restorefn_t
> > __user *__sigrestore_t;
> >
> > Such attribute makes sense for linux since indeed the signal handler
> > code resides in user space and the kernel pointer merely points to
> > user memory here.
> >
> > What is the rationale for this restriction in standard? How we could
> > make clang tolerate such restrictions for X86/BPF/...?
> >
>
> I think we may allow function to be in a non-default address space. The issue is that there may be lots of places in clang or llvm assuming function in default address space, which needs to be fixed.

As John replied in another thread as well, looks like we need to first
fix function pointer usages inside llvm/clang, assuming different
address space, before allowing non-default address space. This makes
sense. Thanks!

>
> > issue 2: typeof(*ptr) where ptr has address_space attribute.
> >
> > -bash-4.4$ cat t2.c
> > int test(int __attribute__((address_space(1))) *p) { #ifdef NO_TYPEOF
> >   int t = *p;
> > #else
> >   typeof(*p) t = *p;
> > #endif
> >   return t;
> > }
> > -bash-4.4$ clang -c t2.c
> > t2.c:5:14: error: automatic variable qualified with an address space
> >   typeof(*p) t = *p;
> >              ^
> > 1 error generated.
>
> Llvm assumes stack address space (alloca address space) is determined by data layout. By default it is 0 but it can be changed by specify a different value in data layout. E.g. amdgcn has alloca address space to be 5. It cannot be explicitly specified in source.

I see. Thanks!

>
> > -bash-4.4$ clang -DNO_TYPEOF -c t2.c
> > -bash-4.4$
> >
> > The IR generated without NO_TYPEOF macro:
> >   %p.addr = alloca i32 addrspace(1)*, align 8
> >   %t = alloca i32, align 4
> >   store i32 addrspace(1)* %p, i32 addrspace(1)** %p.addr, align 8
> >   %0 = load i32 addrspace(1)*, i32 addrspace(1)** %p.addr, align 8
> >   %1 = load i32, i32 addrspace(1)* %0, align 4
> >   store i32 %1, i32* %t, align 4
> >   %2 = load i32, i32* %t, align 4
> >   ret i32 %2
> >
> > So we can directly load a i32 addrspacee(1)* to an i32. Maybe
> > typeof(*p) should just i32 and we should not emit the error at all?
> >
> > Thanks,
> >
> > Yonghong
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Two questions about address_space attribute

Kristof Beyls via cfe-dev
In reply to this post by Kristof Beyls via cfe-dev
Thanks, John, for detailed explanation. Looks like the right way is to
make infrastructure ready first, i.e., making llvm/clang capable to
deal with functions with address space attribute, before we can enable
function pointers with address_space attribute. This makes sense. I
will think how to move forward.

Yonghong

On Tue, Nov 12, 2019 at 8:55 AM John McCall <[hidden email]> wrote:

>
> On 12 Nov 2019, at 11:16, Liu, Yaxun (Sam) via cfe-dev wrote:
>
> + John Matt
>
> My comments are below.
>
> Sam
>
> -----Original Message-----
> From: Y Song <[hidden email]>
> Sent: Tuesday, November 12, 2019 10:15 AM
> To: [hidden email]
> Cc: Andrii Nakryiko <[hidden email]>; Alexei Starovoitov <[hidden email]>; [hidden email]; Liu, Yaxun (Sam) <[hidden email]>
> Subject: Re: Two questions about address_space attribute
>
> [CAUTION: External Email]
>
> Hi, Richard Smith and Yaxun Liu,
>
> You probably have expertise on the two questions below related to address space. Could you comment on my questions below?
>
> Thanks!
>
> Yonghong
>
> On Thu, Nov 7, 2019 at 10:29 AM Y Song <[hidden email]> wrote:
>
> Hi,
>
> Currently, I am thinking to add address_space attribute support for
> x86_64 and BPF to compile linux kernel. The main goal is to get
> address_space information into debug_info and then later to dwarf/BTF.
> But address_space enforcement itself can also help with better code
> for the kernel.
>
> The RFC patch is here:
> https://reviews.llvm.org/D69393
>
> During experiments, we found two issues which I would like to get
> expert opinion:
>
> issue 1: address_space attributes on function pointers
> =========================================
>
> clang does not allow address_space attribute for function pointers,
> specifically, in clang/lib/Sema/SemaType.cpp,
>
> // ISO/IEC TR 18037 S5.3 (amending C99 6.7.3): "A function type shall
> not be // qualified by an address-space qualifier."
> if (Type->isFunctionType()) {
> S.Diag(Attr.getLoc(), diag::err_attribute_address_function_type);
> Attr.setInvalid();
> return;
> }
>
> But linux kernel tries to annotate signal handling function pointer
> with address space to indicate it is accessing user space.
>
> typedef __signalfn_t __user *__sighandler_t; typedef __restorefn_t
> __user *__sigrestore_t;
>
> Such attribute makes sense for linux since indeed the signal handler
> code resides in user space and the kernel pointer merely points to
> user memory here.
>
> What is the rationale for this restriction in standard? How we could
> make clang tolerate such restrictions for X86/BPF/...?
>
> I can’t actually speak for the C committee, but I’ll try to answer anyway. As I understand it, there’s two main things.
>
> The first is that data pointers may already be in a different address space from function pointers. Standard C does not actually allow function pointers to be converted to data pointers and vice-versa; that’s an extension, albeit a basically universal one and indeed one mandated by POSIX because of dlsym. Harvard architectures put code in a separate address space, which is often significantly larger than the address space for data, and C supports that. So while it’s not abstractly illogical for the code space to be split into address spaces, the idea of applying the same address spaces to both data and function pointers isn’t totally aligned with C.
>
> The second is just that the traditional hardware-supported uses of address spaces are centered around data, not code. A processor might have different instructions to load/store from different address spaces, and handling those different address spaces correctly and efficiently is important to expose in a systems programming language. While processors often also have different instructions for e.g. near/far calls, this is rarely useful to expose in the programming language, since it usually only comes up in e.g. the userspace/kernelspace boundary, which is so important and fiddly that’s it’s usually written in assembly anyway.
>
> I think we may allow function to be in a non-default address space. The issue is that there may be lots of places in clang or llvm assuming function in default address space, which needs to be fixed.
>
> Yes, I think LLVM might currently assume a single address space for all code. I would want to see signs of progress on that front before agreeing to an extension of address spaces to function pointer types.
>
> John.
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Two questions about address_space attribute

Kristof Beyls via cfe-dev
In reply to this post by Kristof Beyls via cfe-dev
On Thu, 7 Nov 2019 at 10:30, Y Song via cfe-dev <[hidden email]> wrote:
Hi,

Currently, I am thinking to add address_space attribute support for
x86_64 and BPF to compile linux kernel. The main goal is to get
address_space information into debug_info and then later to dwarf/BTF.
But address_space enforcement itself can also help with better code
for the kernel.

The RFC patch is here:
    https://reviews.llvm.org/D69393

During experiments, we found two issues which I would like to get
expert opinion:

issue 1: address_space attributes on function pointers
=========================================

clang does not allow address_space attribute for function pointers,
specifically, in clang/lib/Sema/SemaType.cpp,

// ISO/IEC TR 18037 S5.3 (amending C99 6.7.3): "A function type shall not be
// qualified by an address-space qualifier."
if (Type->isFunctionType()) {
  S.Diag(Attr.getLoc(), diag::err_attribute_address_function_type);
  Attr.setInvalid();
  return;
}

But linux kernel tries to annotate signal handling function pointer
with address space to indicate it is accessing user space.

typedef __signalfn_t __user *__sighandler_t;
typedef __restorefn_t __user *__sigrestore_t;

Such attribute makes sense for linux since indeed the signal handler
code resides in user space and the kernel pointer
merely points to user memory here.

What is the rationale for this restriction in standard? How we could
make clang tolerate such restrictions for X86/BPF/...?

issue 2: typeof(*ptr) where ptr has address_space attribute.

-bash-4.4$ cat t2.c
int test(int __attribute__((address_space(1))) *p) {
#ifdef NO_TYPEOF
  int t = *p;
#else
  typeof(*p) t = *p;
#endif
  return t;
}
-bash-4.4$ clang -c t2.c
t2.c:5:14: error: automatic variable qualified with an address space
  typeof(*p) t = *p;
             ^
1 error generated.
-bash-4.4$ clang -DNO_TYPEOF -c t2.c
-bash-4.4$

The IR generated without NO_TYPEOF macro:
  %p.addr = alloca i32 addrspace(1)*, align 8
  %t = alloca i32, align 4
  store i32 addrspace(1)* %p, i32 addrspace(1)** %p.addr, align 8
  %0 = load i32 addrspace(1)*, i32 addrspace(1)** %p.addr, align 8
  %1 = load i32, i32 addrspace(1)* %0, align 4
  store i32 %1, i32* %t, align 4
  %2 = load i32, i32* %t, align 4
  ret i32 %2

So we can directly load a i32 addrspacee(1)* to an i32. Maybe
typeof(*p) should just i32 and we should not emit the error at all?

I don't think this suggestion has been fully addressed yet. It's true that we could make typeof(*p) remove the address space qualifier, but that would be inconsistent with how typeof(expr) generally behaves (it doesn't remove type qualifiers from the type of its argument), and it can be important to preserve the address space when the result is used to form another type. For example:

int test(int __attribute__((address_space(1))) *p) {
  const typeof(*p) *const_p = p; // should preserve address_space here

'typeof' is a GNU extension, and generally we want to follow GCC's behavior for GNU extensions, but GCC doesn't implement this attribute (at least not in builds that I've tested), so we have on explicit guidance there. Additionally, it seems wise to keep open the option of specifying an address space on a local variable (even if we don't support it today) for future extensions (such as https://www.springerprofessional.de/en/scads/6859296) rather than ignoring an address space attribute on a local variable.

Can you give an example of code suffering from issue 2? It would help to see how reasonable or unreasonable such code seems to be, and whether this would be best addressed in the compiler or in the code.

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Two questions about address_space attribute

Kristof Beyls via cfe-dev
On Tue, Nov 12, 2019 at 4:21 PM Richard Smith <[hidden email]> wrote:

>
> On Thu, 7 Nov 2019 at 10:30, Y Song via cfe-dev <[hidden email]> wrote:
>>
>> Hi,
>>
>> Currently, I am thinking to add address_space attribute support for
>> x86_64 and BPF to compile linux kernel. The main goal is to get
>> address_space information into debug_info and then later to dwarf/BTF.
>> But address_space enforcement itself can also help with better code
>> for the kernel.
>>
>> The RFC patch is here:
>>     https://reviews.llvm.org/D69393
>>
>> During experiments, we found two issues which I would like to get
>> expert opinion:
>>
>> issue 1: address_space attributes on function pointers
>> =========================================
>>
>> clang does not allow address_space attribute for function pointers,
>> specifically, in clang/lib/Sema/SemaType.cpp,
>>
>> // ISO/IEC TR 18037 S5.3 (amending C99 6.7.3): "A function type shall not be
>> // qualified by an address-space qualifier."
>> if (Type->isFunctionType()) {
>>   S.Diag(Attr.getLoc(), diag::err_attribute_address_function_type);
>>   Attr.setInvalid();
>>   return;
>> }
>>
>> But linux kernel tries to annotate signal handling function pointer
>> with address space to indicate it is accessing user space.
>>
>> typedef __signalfn_t __user *__sighandler_t;
>> typedef __restorefn_t __user *__sigrestore_t;
>>
>> Such attribute makes sense for linux since indeed the signal handler
>> code resides in user space and the kernel pointer
>> merely points to user memory here.
>>
>> What is the rationale for this restriction in standard? How we could
>> make clang tolerate such restrictions for X86/BPF/...?
>>
>> issue 2: typeof(*ptr) where ptr has address_space attribute.
>>
>> -bash-4.4$ cat t2.c
>> int test(int __attribute__((address_space(1))) *p) {
>> #ifdef NO_TYPEOF
>>   int t = *p;
>> #else
>>   typeof(*p) t = *p;
>> #endif
>>   return t;
>> }
>> -bash-4.4$ clang -c t2.c
>> t2.c:5:14: error: automatic variable qualified with an address space
>>   typeof(*p) t = *p;
>>              ^
>> 1 error generated.
>> -bash-4.4$ clang -DNO_TYPEOF -c t2.c
>> -bash-4.4$
>>
>> The IR generated without NO_TYPEOF macro:
>>   %p.addr = alloca i32 addrspace(1)*, align 8
>>   %t = alloca i32, align 4
>>   store i32 addrspace(1)* %p, i32 addrspace(1)** %p.addr, align 8
>>   %0 = load i32 addrspace(1)*, i32 addrspace(1)** %p.addr, align 8
>>   %1 = load i32, i32 addrspace(1)* %0, align 4
>>   store i32 %1, i32* %t, align 4
>>   %2 = load i32, i32* %t, align 4
>>   ret i32 %2
>>
>> So we can directly load a i32 addrspacee(1)* to an i32. Maybe
>> typeof(*p) should just i32 and we should not emit the error at all?
>
>
> I don't think this suggestion has been fully addressed yet. It's true that we could make typeof(*p) remove the address space qualifier, but that would be inconsistent with how typeof(expr) generally behaves (it doesn't remove type qualifiers from the type of its argument), and it can be important to preserve the address space when the result is used to form another type. For example:
>
> int test(int __attribute__((address_space(1))) *p) {
>   const typeof(*p) *const_p = p; // should preserve address_space here
>
> 'typeof' is a GNU extension, and generally we want to follow GCC's behavior for GNU extensions, but GCC doesn't implement this attribute (at least not in builds that I've tested), so we have on explicit guidance there. Additionally, it seems wise to keep open the option of specifying an address space on a local variable (even if we don't support it today) for future extensions (such as https://www.springerprofessional.de/en/scads/6859296) rather than ignoring an address space attribute on a local variable.
>
> Can you give an example of code suffering from issue 2? It would help to see how reasonable or unreasonable such code seems to be, and whether this would be best addressed in the compiler or in the code.

The following is our use case.
The linux kernel has a macro "put_user" defined to write a simple
value to user space
  #define put_user(x, ptr)

where x is the value and ptr is a pointer to user space. For example,

   int __attribute__((address_space(1))) *optlen;
   int len = ...;
   ... put_user(len, optlen) ...

FYI, the source code link:
https://github.com/torvalds/linux/blob/master/arch/x86/include/asm/uaccess.h#L228-L244

The put_user is defined like below:

#define put_user(x, ptr)                                        \
({                                                              \
        int __ret_pu;                                           \
        __typeof__(*(ptr)) __pu_val;                            \
.....

The error will happen for the above
   __typeof__(*(ptr)) __pu_val;
as the ptr has __attribute__((address_space(1))) annotation.

The put_user macro is used many many times in kernel. I would like to
avoid changing
its signature, hence I am asking question here.
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Two questions about address_space attribute

Kristof Beyls via cfe-dev
In reply to this post by Kristof Beyls via cfe-dev
Regarding the Issue 1 - what are you trying to achieve by qualifying function pointers with address spaces? Ideally functions are placed by an implementation.

Cheers,
Anastasia

From: cfe-dev <[hidden email]> on behalf of Y Song via cfe-dev <[hidden email]>
Sent: 07 November 2019 18:29
To: [hidden email] <[hidden email]>
Cc: Andrii Nakryiko <[hidden email]>; Alexei Starovoitov <[hidden email]>
Subject: [cfe-dev] Two questions about address_space attribute
 
Hi,

Currently, I am thinking to add address_space attribute support for
x86_64 and BPF to compile linux kernel. The main goal is to get
address_space information into debug_info and then later to dwarf/BTF.
But address_space enforcement itself can also help with better code
for the kernel.

The RFC patch is here:
    https://reviews.llvm.org/D69393

During experiments, we found two issues which I would like to get
expert opinion:

issue 1: address_space attributes on function pointers
=========================================

clang does not allow address_space attribute for function pointers,
specifically, in clang/lib/Sema/SemaType.cpp,

// ISO/IEC TR 18037 S5.3 (amending C99 6.7.3): "A function type shall not be
// qualified by an address-space qualifier."
if (Type->isFunctionType()) {
  S.Diag(Attr.getLoc(), diag::err_attribute_address_function_type);
  Attr.setInvalid();
  return;
}

But linux kernel tries to annotate signal handling function pointer
with address space to indicate it is accessing user space.

typedef __signalfn_t __user *__sighandler_t;
typedef __restorefn_t __user *__sigrestore_t;

Such attribute makes sense for linux since indeed the signal handler
code resides in user space and the kernel pointer
merely points to user memory here.

What is the rationale for this restriction in standard? How we could
make clang tolerate such restrictions for X86/BPF/...?

issue 2: typeof(*ptr) where ptr has address_space attribute.

-bash-4.4$ cat t2.c
int test(int __attribute__((address_space(1))) *p) {
#ifdef NO_TYPEOF
  int t = *p;
#else
  typeof(*p) t = *p;
#endif
  return t;
}
-bash-4.4$ clang -c t2.c
t2.c:5:14: error: automatic variable qualified with an address space
  typeof(*p) t = *p;
             ^
1 error generated.
-bash-4.4$ clang -DNO_TYPEOF -c t2.c
-bash-4.4$

The IR generated without NO_TYPEOF macro:
  %p.addr = alloca i32 addrspace(1)*, align 8
  %t = alloca i32, align 4
  store i32 addrspace(1)* %p, i32 addrspace(1)** %p.addr, align 8
  %0 = load i32 addrspace(1)*, i32 addrspace(1)** %p.addr, align 8
  %1 = load i32, i32 addrspace(1)* %0, align 4
  store i32 %1, i32* %t, align 4
  %2 = load i32, i32* %t, align 4
  ret i32 %2

So we can directly load a i32 addrspacee(1)* to an i32. Maybe
typeof(*p) should just i32 and we should not emit the error at all?

Thanks,

Yonghong
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev