[PATCH] D123198: [LibCalls] Add argument extension attributes to more functions.

[Dávid Bolvanský via Phabricator via llvm-commits]

xbolva00 added a comment.

In D123198#3442063 <https://reviews.llvm.org/D123198#3442063>, @jonpa wrote:

>> BuildLibCalls does not run under -O0 afaik .. so you wont get these attrs. So I dont think this is a correct place if my assumption holds.
>
> I am quite sure there are more places to handle, but fixing BuildLibCalls seems like a step in the right direction, wouldn't you agree? This is an existing problem we are trying to fix by carefully finding and handling all call sites throughout the code base.

I dont stiĺ think this is a correct place.

If this attribute is required for correctness, you should write simple pass which adds this atrribute very soon in pipeline and runs even under -O0. Or @efriedma may propose something…

>> And I also think that things will break for you if somebody uses eg. -fno-builtin-memccpy .. you will not get this ext attribute.
>
> I would be very grateful if you helped with this work by filing bug reports with test cases, and maybe even patches...

This is by design - if -ifno-builtin-xxx is used, a libcall is not annotated with any attribute here. This works fine and you are misusing BuildLibCalls and the solution is not to change when and how BuildLibCalls runs.

In D123198#3442133 <https://reviews.llvm.org/D123198#3442133>, @jonpa wrote:

>> I dont stiĺ think this is a correct place.
>>
>> If this attribute is required for correctness, you should write simple pass which adds this atrribute very soon in pipeline and runs even under -O0. Or @efriedma may propose something…
>
> It is not a question if it is required for correctness or not (even though not all targets have this in their ABIs). The SystemZ Clang Front End does this already, so there should not be any need for an early pass to do this. Basically all front ends emitting code for SystemZ are obliged to annotate call parameters with the proper extension attribute, but that is not what this patch is handling.
>
>> This is by design - if -ifno-builtin-xxx is used, a libcall is not annotated with any attribute here. This works fine and you are misusing BuildLibCalls and the solution is not to change when and how BuildLibCalls runs.
>
> I would hope that with -fno-builtin the original call would be already be correctly emitted by Clang (if not, that's the place to fix it probably).
>
> I don't understand your objections to this patch. I am not changing when or how BuildLibCalls is used, merely making sure it adds the needed parameter attributes for the targets that need it. If your concern is about chown() and lchown(), let's see what Eli has to say about that...

Thanks, it makes more sense now. But info like this info should be in patch description to better understand the whole problem.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123198/new/

https://reviews.llvm.org/D123198