Re: [PATCH] fs: remove implicit compiler calls to memset/memcpy

[Andrei Borzenkov]

On Mon, Apr 18, 2016 at 1:13 PM, Pete Batard <address@hidden> wrote:
> On 2016.04.18 07:50, Vladimir 'phcoder' Serbinenko wrote:
>>
>> You can use asm to get around msvc limitations. Sth like
>>
>> .global memcpy
>> memcpy:
>>        jmp grub_memcpy
>
>
> Yes I'm well aware I could try to create my own library (or equivalent) that
> redefines memcpy/memset, using some workaround to avoid the MSVC compiler
> error (if needed, I could create a compatible aliasing lib with gcc). But
> that's something I'd prefer to avoid because it's never that simple in the
> MSVC world, and I'm also compiling for multiple archs (x86 and ARM, possibly
> more in the future), which makes assembly workarounds an annoyance. Besides,

grub so far was relatively good at localizing platform and compiler
specific code. It sounds like something that fits well.

> I already have to patch the GRUB source anyway, with some MSVC specific
> quirks (some of which I doubt you guys would like [1]), so adding some more
> to my current patch series to remove the implicits is not exactly a big
> deal. But of course, I sure wouldn't mind minimizing the amount of code I
> need to patch to use GRUB in my scope, which is the whole point of this
> submission.
>
>> Where implicit memcpy is inserted is pretty much unpredictable
>
>
> You're missing the purpose of my request. I'm not asking the GRUB project to
> do anything about predicting where memset/memcpy are going inserted, or even
> attempt to be preemptive about that in the future. I'm simply asking it to
> address the ones that were isolated with 100% accuracy (compiler flag to
> generate assembly with source + grep on said assembly) from using the
> current GRUB source in the specific context that I have.
> There's no "predictive" component in what I am requesting here, not even an
> implied one.
>

Compilers quite likely avoid calling external functions in this case;
so it prevents optimizing code on other platforms/compilers.

> So, in case that is the crux of the issue, should this patch be integrated,
> I am not asking any GRUB contributor to try to keep conscious of or try to
> identify potential implicits as they go about modifying fs or any other
> parts of the code. It is in fact of little consequence if someone comes in
> and breaks the applied implicits removal the day after it is applied, as I
> am using GRUB as a git submodule, therefore it'll remain tied to the
> specific git rev where it isn't broken. And if I try to update the
> submodule, and identify breakage, I will of course submit a new patch to
> this list as needed.
>
>> and we're
>> not going to maintain memcpy-free environment because of this
>
>
> Again, this is not what I am asking. This is a simple "you scratch my back,
> I'll scratch yours" request, that, in a way, is pretty much akin to asking
> the GRUB project to add extra parenthesis in _very specific_ parts of
> source, to make it more palatable when these specific parts are used with
> compilers that are not officially supported by the project, and as a one-off
> thing (i.e. without asking for anyone to be tasked to maintaining that new
> parenthesis layout going forward).
>
> If you want to say: "Well, we don't official support MSVC, so we're not
> going to pick a patch that is essentially aimed at improving MSVC support,
> especially if only applies to a limited set of sources", that's fine with
> me.

Personally I would welcome any patch aimed at improving MSVC support
as long as it is not added at expense of another platforms. We do
support native grub on Windows so it is just logical to support native
Windows build environment. It is just that no current grub maintainers
is using MSVC so if you volunteer to step in it would be great.

> But please don't try to imply that the patch has a much larger scope than it
> actually does, as your justification for rejection, or that it is going to
> require any extra work/maintainance from GRUB contributors once applied.
>
> Regards,
>
> /Pete
>
> [1]
> https://github.com/pbatard/efifs/blob/master/0001-GRUB-fixes-for-MSVC-compilation-part-1.patch#L366-L410
>
>
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel