[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v6 1/4] xen: introduce SYMBOL
Hi Jan, On 1/15/19 8:21 AM, Jan Beulich wrote: On 14.01.19 at 22:18, <sstabellini@xxxxxxxxxx> wrote:Hi Jan, One question below to make a decision on the way forward. On Mon, 14 Jan 2019, Jan Beulich wrote:On 14.01.19 at 04:45, <Stewart.Hildebrand@xxxxxxxxxxxxxxx> wrote:So let's keep the linker-accessible variable as a type that works for the linker (which really could be anything as long as you use the address, not the value), but name it something else - a name that screams "DON'T USE ME UNLESS YOU KNOW WHAT YOU'RE DOING". And then before the first use, copy that value to "uintptr_t _start;". The following is a quick proof of concept for aarch64. I changed the type of _start and _end, and added code to copy the linker-assigned value to _start and _end. Upon booting, I see the correct values:Global symbols starting with underscores should already be shouting enough. But what's worse - the whole idea if using array types is to avoid the intermediate variables.--- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -726,6 +726,12 @@ static void __init setup_mm(unsigned long dtb_paddr,size_t dtb_size)size_t __read_mostly dcache_line_bytes; +typedef char TYPE_DOESNT_MATTER;+extern TYPE_DOESNT_MATTER _start_linker_assigned_dont_use_me, + _end_linker_assigned_dont_use_me;This and ...@@ -770,10 +776,17 @@ void __init start_xen(unsigned long boot_phys_offset, printk("Command line: %s\n", cmdline); cmdline_parse(cmdline);+ _start = (uintptr_t)&_start_linker_assigned_dont_use_me;+ _end = (uintptr_t)&_end_linker_assigned_dont_use_me;... this violates what the symbol names say. And if you want to avoid issues, you'd want to keep out of C files uses of those symbols altogether anyway, and you easily can: In any assembly file, have _start: .long _start_linker_assigned_dont_use_me _end: .long _end_linker_assigned_dont_use_me In particular, they don't need to be runtime initialized, saving you from needing to set them before first use. But as said - things are the way they are precisely to avoid such variables.But, instead of converting _start to unsigned long via SYMBOL_HIDE, we could convert it to uintptr_t instead, it would be a trivial change on top of the existing unsigned long series. Not sure if it is beneficial.The difference would be whether we want to rely on implementation-defined behavior or not.Why not? Simply specify that compilers with implementation defined behavior not matching our expectations are unsuitable. And btw, I suppose this is just the tiny tip of the iceberg of our reliance on implementation defined behavior.The reason is that relying on undefined behavior is not reliable, it is not C compliant, it is not allowed by MISRA-C, and not guaranteed to work with any compiler."undefined behavior" != "implementation defined behavior"Yes, this instance is only the tip of the iceberg, we have a long road ahead, but we shouldn't really give up because it is going to be difficult :-) Stewart's approach would actually be compliant and help toward reducing reliance on undefined behavior. Would you be OK if I rework the series to follow his approach using intermediate variables? See the attached patch as a reference, it only "converts" _start and _end as an example. Fortunately, it will be textually similar to the previous SYMBOL returning unsigned long version of the series.Well, I've given reasons why I dislike that, and why (I think) it was done without such intermediate variables. Nevertheless, if this is _the only way_ to achieve compliance, I don't think I could reasonably NAK it. > The thing that I don't understand though is how the undefined behavior (if there really is any) goes away: Even if you compare the contents of the variables instead of the original (perhaps casted) pointers, in the end you still compare what C would consider pointers to different objects. It's merely a different way of hiding that fact from C. Undefined behavior would imo go away only if those comparisons/subtractions didn't happen in C anymore. IOW - see my .startof.() / .sizeof.() proposal. Do you have a pointer to the series using startof/sizeof? If you are OK with it, do you have any suggestions on how would you like the intermediate variables to be called? I went with _start/start_ and _end/end_ but I am open to suggestions. Also to which assembly file you would like the new variables being added -- I created a new one for the purpose named var.S in the attached example.First of all we should explore whether the variables could also be linker generated, in particular to avoid the current symbols to be global (thus making it impossible to access them from C files in the first place). Failing that, I don't think it matters much where these helper symbols live, and hence your choice is probably fine (I'd prefer though if, just like on Arm, the x86 file didn't live in the boot/ subdirectory; in the end it might even be possible to have some of them in xen/common/var.S). From my test [1], I don't think intermediate variables are necessary. You could directly define the symbol with uintptr_t. Cheers, [1] https://lists.xen.org/archives/html/xen-devel/2019-01/msg01109.html Jan -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |