[mpich-discuss] Incorrect error checking?
Raffenetti, Kenneth J.
raffenet at mcs.anl.gov
Thu Jun 20 14:38:50 CDT 2019
FWIW, the example below does generate a warning with clang. The
unaligned access would probably cause a SIGBUS on sparc.
$ clang foo.c
foo.c:14:18: warning: taking address of packed member 'b' of class or
structure '' may result in
an unaligned pointer value [-Waddress-of-packed-member]
c = (int *)&s.b;
^~~
1 warning generated.
Still, the check is dependent on a non-cross-compile safe configure test.
Ken
On 6/20/19 2:09 PM, Raffenetti, Kenneth J. via discuss wrote:
> Hmm, interesting. I think this example false-positive is pretty good
> reason to remove the check from MPICH.
>
> Ken
>
> On 6/20/19 1:42 PM, Nick Radcliffe via discuss wrote:
>> Hui,
>>
>>
>> The library was built on one of our internal build servers, and run on a
>> different machine of course. But I don't think it's a configure issue. I
>> did a quick test and it's definitely possible for the address of a
>> pointer to not be 8-byte aligned:
>>
>>
>> [nradclif at heron ~]$ cat test_align.c
>> #include <stdlib.h>
>> #include <stdio.h>
>>
>> int main()
>> {
>> int *c;
>>
>> struct
>> {
>> int a;
>> int *b;
>> } __attribute__((packed)) s;
>>
>> c = (int *)&s.b;
>>
>> fprintf(stdout, "c = %p\n", c);
>>
>> return 0;
>> }
>> [nradclif at heron ~]$ ./test_align.x
>> c = 0x7ffe3d4e2c44
>>
>>
>> I'll have to talk to the user to get more details about their code.
>>
>>
>>
>> Nick Radcliffe Software Engineer| Cray Inc.
>>
>> 2131 Lindau Ln #1000 | Bloomington, MN55425
>>
>> +1-651-605-8864 nradclif at cray.com <mailto:email at cray.com> www.cray.com
>> <http://www.cray.com>
>>
>> ------------------------------------------------------------------------
>> *From:* Zhou, Hui <zhouh at anl.gov>
>> *Sent:* Thursday, June 20, 2019 1:13:43 PM
>> *To:* discuss at mpich.org
>> *Cc:* Nick Radcliffe
>> *Subject:* Re: [mpich-discuss] Incorrect error checking?
>> That’s interesting. It means the `configure` was finding pointers has to
>> 8-bytes aligned but the user code is having pointers that are 4-byte
>> aligned. I suspect the user did not build the library on the same
>> machine it runs? I am also curious that which architecture actually are
>> not allowing the 4-byte aligned pointers. Nick, is it possible to trace
>> where the questioned mpi library was configured?
>>
>> —
>> Hui Zhou
>> T: 630-252-3430
>>
>>
>>
>>
>>
>>
>>
>>
>>> On Jun 20, 2019, at 1:04 PM, Nick Radcliffe via discuss
>>> <discuss at mpich.org <mailto:discuss at mpich.org>> wrote:
>>>
>>> Ken,
>>>
>>> The reason I brought this up is that a user's (valid) code was failing
>>> due to this parameter check. It's possible to use an address that's
>>> not 8 byte aligned to store a pointer to an int, i.e.,
>>>
>>> int *x;
>>> int *y;
>>>
>>> x = malloc(16);
>>> y = &x[1];
>>>
>>> I would generally expect y to be 4 byte aligned in the above case. So
>>> sometimes this parameter check will cause a correct program to fail,
>>> right?
>>>
>>>
>>> Nick Radcliffe Software Engineer| Cray Inc.
>>> 2131 Lindau Ln #1000|Bloomington,MN55425
>>> +1-651-605-8864 nradclif at cray.com
>>> <mailto:email at cray.com> www.cray.com <http://www.cray.com/>
>>>
>>> ------------------------------------------------------------------------
>>> *From:*Raffenetti, Kenneth J. via discuss <discuss at mpich.org
>>> <mailto:discuss at mpich.org>>
>>> *Sent:*Wednesday, June 19, 2019 2:44:40 PM
>>> *To:*discuss at mpich.org <mailto:discuss at mpich.org>
>>> *Cc:*Raffenetti, Kenneth J.
>>> *Subject:*Re: [mpich-discuss] Incorrect error checking?
>>> Upon further thought, it's just trying to catch a common error, and uses
>>> alignment to detect it since the type information is lost thru the
>>> interface. In testing on x86_64, it doesn't appear that it will catch
>>> the intended error.
>>>
>>> As Thomas pointed out in another reply, there are also some issues with
>>> the implementation, though it is correct that we require intptr_t to
>>> build MPICH. At the very least, we could patch to use sizeof(void *) vs.
>>> sizeof(intptr_t). Or we can still remove the check since the use-case is
>>> so narrow. The preprocessor guard also depends on a AC_TRY_RUN that I'd
>>> prefer to delete, to be honest.
>>>
>>> Ken
>>>
>>> On 6/19/19 9:27 AM, Raffenetti, Kenneth J. via discuss wrote:
>>>> Nick,
>>>>
>>>> I agree this check looks like nonsense. IMO, we can just delete it and
>>>> the others like it. I will create a PR.
>>>>
>>>> Ken
>>>>
>>>> On 6/18/19 5:25 PM, Nick Radcliffe via discuss wrote:
>>>>> I recently noticed that a parameter check in MPII_Win_get_attr looks a
>>>>> bit off:
>>>>>
>>>>>
>>>>> /* A common user error is to pass the address of a 4-byte
>>>>> * int when the address of a pointer (or an address-sized int)
>>>>> * should have been used. We can test for this specific
>>>>> * case. Note that this code assumes sizeof(intptr_t) is
>>>>> * a power of 2. */
>>>>> if ((intptr_t) attribute_val & (sizeof(intptr_t) - 1)) {
>>>>> MPIR_ERR_SETANDSTMT(mpi_errno, MPI_ERR_ARG, goto
>>>>> fn_fail, "**attrnotptr");
>>>>> }
>>>>>
>>>>>
>>>>> The comment indicates that the check is testing "attribute_val" to see
>>>>> if a pointer to int was passed in, rather than a double pointer or
>>>>> pointer to an integer that can store an address. But the check seems to
>>>>> only be testing for 8 byte alignment (and failing if not aligned). Am I
>>>>> missing something here?
>>>>>
>>>>>
>>>>>
>>>>> Nick Radcliffe Software Engineer| Cray Inc.
>>>>>
>>>>> 2131 Lindau Ln #1000 | Bloomington, MN55425
>>>>>
>>>>> +1-651-605-8864 nradclif at cray.com <http://cray.com> <mailto:email at cray.com> www.cray.com
>>>>> <http://www.cray.com <http://www.cray.com/>>
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> discuss mailing list discuss at mpich.org <mailto:discuss at mpich.org>
>>>>> To manage subscription options or unsubscribe:
>>>>> https://lists.mpich.org/mailman/listinfo/discuss
>>>>>
>>>> _______________________________________________
>>>> discuss mailing list discuss at mpich.org <mailto:discuss at mpich.org>
>>>> To manage subscription options or unsubscribe:
>>>> https://lists.mpich.org/mailman/listinfo/discuss
>>>>
>>> _______________________________________________
>>> discuss mailing list discuss at mpich.org <mailto:discuss at mpich.org>
>>> To manage subscription options or unsubscribe:
>>> https://lists.mpich.org/mailman/listinfo/discuss
>>> _______________________________________________
>>> discuss mailing list discuss at mpich.org <mailto:discuss at mpich.org>
>>> To manage subscription options or unsubscribe:
>>> https://lists.mpich.org/mailman/listinfo/discuss
>>
>>
>> _______________________________________________
>> discuss mailing list discuss at mpich.org
>> To manage subscription options or unsubscribe:
>> https://lists.mpich.org/mailman/listinfo/discuss
>>
> _______________________________________________
> discuss mailing list discuss at mpich.org
> To manage subscription options or unsubscribe:
> https://lists.mpich.org/mailman/listinfo/discuss
>
More information about the discuss
mailing list