This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH V2 2/8] bpf: new GCC port
Hi Richard!
Many thanks for the deep review. I'm addressing some of your questions
below.
> [...]
> +/* Override options and do some other initialization. */
> +
> +static void
> +bpf_option_override (void)
> +{
> + /* Set the default target kernel if no -mkernel was specified. */
> + if (!global_options_set.x_bpf_kernel)
> + bpf_kernel = LINUX_LATEST;
LINUX_LATEST is the default in the .opt file, so when is this needed?
It is an idiom I got from sparc.c:
/* Set the default CPU if no -mcpu option was specified. */
if (!global_options_set.x_sparc_cpu_and_features)
{
...
}
Maybe the code in sparc.c predates the Init() directive in sparc.opt
file?
> +/* Determine whether the port is prepared to handle insns involving
> + scalar mode MODE. For a scalar mode to be considered supported,
> + all the basic arithmetic and comparisons must work. */
> +
> +static bool
> +bpf_scalar_mode_supported_p (scalar_mode mode)
> +{
> + switch (mode)
> + {
> + case E_QImode:
> + case E_HImode:
> + case E_SImode:
> + case E_DImode:
> + case E_TImode:
> + return true;
> +
> + default:
> + return false;
> + }
> +
> + return false;
> +}
Are you overriding this just to exclude floating-point modes?
If so, what specifically doesn't work?
Would be worth a comment.
Reminiscence of not having support for TImodes at some stage. I'm
removing the target hook.
> [...]
> +/* Return true if a value of mode MODE1 is accessible in mode MODE2
> + without copying. */
> +
> +static bool
> +bpf_modes_tieable_p (enum machine_mode mode1,
> + enum machine_mode mode2)
> +{
> + return (mode1 == mode2
> + || GET_MODE_CLASS (mode1) == GET_MODE_CLASS (mode2));
> +}
The second part makes the first part redundant. But why do you
need to restrict it based on classes? It seems like eBPF is an
example of an architecture where any modes are tieable, so using
the default would be better if possible.
If the restriction is needed, it would be worth having a comment
explaining which case you're excluding and why.
Hm yes, you are right. TARGET_HARD_REGNO_MODE_OK (R,MODE1) ==
TARGET_HARD_REGNO_MODE_OK (R,MODE2) for all supported modes... I'm
removing the target hook.
> [...]
> + case PLUS:
> + {
> + /* The valid patterns here are:
> +
> + (PLUS ADDR_BASE CONST_INT)
> + (PLUS CONST_INT ADDR_BASE)
The second one isn't canonical rtl, so you shouldn't (need to) handle it.
Please raise a bug if you find a case where it's being generated. :-)
Oooh, didn't know that, that's actually very handy :)
Do you know if this is documented anywhere? I don't recall seeing this
in the internals manual, but maybe I missed it.
> [...]
> +/* Split an out-of-range address displacement into hi and lo parts.
> + The hi part will have to be loaded into a register separately, but
> + the low part will be folded into the memory operand. */
> +
> +static bool
> +bpf_legitimize_address_displacement (rtx *off1, rtx *off2,
> + poly_int64 poly_offset, machine_mode)
> +{
> + HOST_WIDE_INT orig_offset = poly_offset;
> +
> + /* Our case is very easy: the REG part of an indirect address is
> + 64-bit wide, so it can hold any address. This always leads to
> + REG+0 */
> +
> + *off1 = GEN_INT (orig_offset);
> + *off2 = GEN_INT (0);
> + return true;
> +}
> +
> +#undef TARGET_LEGITIMIZE_ADDRESS_DISPLACEMENT
> +#define TARGET_LEGITIMIZE_ADDRESS_DISPLACEMENT bpf_legitimize_address_displacement
But then do you need to define the hook at all? I think this is what
LRA does by default. The hook is only really there to get better spill
code (via common anchor points) on targets with limited offset ranges.
Hm I think I wrote this hook to fix some invalid addresses being
generated at some point... but I don't recall the details. It looks
like this hook is no longer necessary with the current implementation of
addresses (legitimize etc) so I'm removing it.
> [...]
> +/* Return true if memory address ADDR in address space AS can have
> + different meanings depending on the machine mode of the memory
> + reference it is used for or if the address is valid for some modes
> + but not others. */
> +
> +static bool
> +bpf_mode_dependent_address_p (const_rtx addr ATTRIBUTE_UNUSED,
> + addr_space_t as ATTRIBUTE_UNUSED)
> +{
> + return true;
> +}
> +
> +#undef TARGET_MODE_DEPENDENT_ADDRESS_P
> +#define TARGET_MODE_DEPENDENT_ADDRESS_P bpf_mode_dependent_address_p
Why does this need to be true? False is the better answer if you can
give it. :-) And it looks like the set of legitimate addresses doesn't
really care about modes.
That true was supposed to be false! :)
I'm removing the hook, as the default returns false anyway.
> [...]
> +/* Return a RTX indicating whether a function argument is passed in a
> + register and if so, which register. */
> +
> +static rtx
> +bpf_function_arg (cumulative_args_t ca, enum machine_mode mode ATTRIBUTE_UNUSED,
> + const_tree type ATTRIBUTE_UNUSED, bool named ATTRIBUTE_UNUSED)
> +{
> + CUMULATIVE_ARGS *cum = get_cumulative_args (ca);
> +
> + if (*cum < 5)
> + return gen_rtx_REG (mode, *cum + 1);
> + else
> + /* An error have been emitted for this in
> + bpf_function_arg_advance. */
> + return NULL_RTX;
This hook is called first, so "will be" rather than "has been".
(BTW, I just submitted a series of patches to change this interface,
but it should be a trivial change for whichever of us gets to make it.)
I see your interface change was approved yesterday, so I will just adapt
in my next rebase :)
> [...]
> +/* Return true if an argument at the position indicated by CUM should
> + be passed by reference. If the hook returns true, a copy of that
> + argument is made in memory and a pointer to the argument is passed
> + instead of the argument itself. */
> +
> +static bool
> +bpf_pass_by_reference (cumulative_args_t cum ATTRIBUTE_UNUSED,
> + enum machine_mode mode, const_tree type,
> + bool named ATTRIBUTE_UNUSED)
> +{
> + unsigned HOST_WIDE_INT size;
> +
> + if (type)
> + {
> + if (AGGREGATE_TYPE_P (type))
> + return true;
> + size = int_size_in_bytes (type);
> + }
> + else
> + size = GET_MODE_SIZE (mode);
> +
> + return (size > 8*5);
> +}
> +
> +#undef TARGET_PASS_BY_REFERENCE
> +#define TARGET_PASS_BY_REFERENCE bpf_pass_by_reference
I might have misunderstood, but I thought from an earlier (IRC?)
message, it wasn't possible for the callee to access the caller's
frame, which was why you had the error about running out of argument
registers. If so, won't passing by reference make the argument
inaccessible in practice? I don't see what you gain by defining
the hook, since I'd have assumed (at least after the fix above)
that it would be better to pass by value and get an error about
having no argument registers left.
Yes. I added that hook before I had the restriction of number of
arguments in place. Removing it.
> [...]
> +/* Always promote arguments and return values in function calls. */
> +
> +#undef TARGET_PROMOTE_FUNCTION_MODE
> +#define TARGET_PROMOTE_FUNCTION_MODE default_promote_function_mode_always_promote
Not a review comment, just curious: why did you go for this choice?
Is it already enshrined in the ABI?
One of the historical problems with caller promotion from a security
perspective is that the callee then trusts the caller to do the right
thing. See e.g. the code in combine.c that optimises away "redundant"
extensions that the caller is assumed to have done for us.
E.g. a rogue caller could induce an out-of-bounds access for:
unsigned int a[256];
unsigned int f (unsigned char c) { return a[c]; }
because the zero-extension of "c" in the address calculation might be
optimised away. This might not matter in an eBPF context though...
Interesting. I have to think about this, and also check whether llvm is
doing caller argument promotion or not.
> [...]
> +/* The widest floating-point format supported by the hardware is
> + 64-bit. */
> +#define WIDEST_HARDWARE_FP_SIZE 64
Normally soft-fp targets don't need to define this. Is this related
to the special conversion libcalls you install for eBPF?
No. I didn't realize its value defaults to LONG_DOUBLE_TYPE_SIZE.
Removing the definition.