January 2014 - devel - mailman3.cels.anl.gov

Re: [mpich-devel] [mpich-discuss] Use of Signed-off-by in MPICH
by Michael Blocksome 06 Jan '14

06 Jan '14

In our workflow, often the commit is pushed to an ibm review branch, then the signer (usually me) does a signed-off cherry-pick to an anl review branch and pushes the code to mpich-ibm git repository for ANL to integrate. Using "signed off by" is usually much easier than playing git games to ensure that the author and committer are different. However, it really doesn't matter to IBM. We would prefer to follow a documented process that is used by ANL mpich team, but as of now I am not aware of any such documentation. The goal from our perspective is to ensure that an internal code review has been done before passing the code of to ANL - and it would be nice to know who at ANL reviewed the code before it was pushed to master. Michael Blocksome Parallel Environment MPI Middleware Team Lead, TCEM POWER, x86, and Blue Gene HPC Messaging blocksom(a)us.ibm.com From: Jed Brown <jedbrown(a)mcs.anl.gov> To: Michael Blocksome/Rochester/IBM@IBMUS, Cc: devel(a)mpich.org Date: 01/06/2014 01:21 PM Subject: Re: [mpich-discuss] Use of Signed-off-by in MPICH Michael Blocksome <blocksom(a)us.ibm.com> writes: > IBM uses the "signed off by" feature to enforce/track code reviews before > any code gets contributed to ANL. The intent is that the commit author != > the commit signer when the code goes out. Often a developer at ANL will > also review the code before it gets integrated into the master branch (IBM > developers don't have push authority) and sign the commit again. So the last stage (whoever makes the actual commit) is already available because Git has separate fields for Committer and Author, as shown by "git log --format=fuller". > We use the text "Signed-off-by:xxxx <yyy at zzz>" simply because this is what > is supported by the git tools. You can use commands like `git cherry-pick > -s`, `git commit --amend -s`, and `git am -s` to ease the signing effort. Yes, though this only adds your own name. Do you email patches around until then? When I started this thread, I pointed out a recent case where the author and committer were the same, but someone else was providing a Signed-off-by. Either the commit went back and forth or (more likely) the committer amended the commit to add a Signed-off-by >From the reviewer. The idea with Git's -s is that you only ever sign for yourself, though if you integrate out-of-band content, you may copy the Signed-off-by tag provided by that other person. So the design of the -s option is not a perfect match, but I can see that it's often convenient. In contrast, Reviewed-by, Acked-by, and similar are almost always a result of the author or committer translating out-of-band information (e.g., an email discussion). If you want to automatically add Reviewed-by tags, you could use .git/hooks/prepare-commit-msg, for example. Of course you can keep using Signed-off-by in place of Reviewed-by, though it causes some confusion for people new to the project, attempting to track provenance when either extracting code or using submodules with other projects that use the usual conventions, or scripts providing repository analytics.

1 0

Re: [mpich-devel] build broken on 'master': 'mpitypedefs.h' file not found
by Jed Brown 06 Jan '14

06 Jan '14

Rob Latham <robl(a)mcs.anl.gov> writes: > I think you're looking for the opposite question. > MPI_AINT_CAST_TO_VOID_PTR is used throughout the MPICH code base. > ROMIO should use MPI_AINT_CAST_TO_VOID_PTR, providing it's own (stubed > out?) version if that macro is not available. You're calling MPI_AINT_CAST_TO_VOID_PTR from glue_romio.h. I don't know which way all your dependencies run, but why not have all the ROMIO code use a ROMIO version of the macro (something that is always available)?

1 0

Re: [mpich-devel] Weak symbols, clang, and users compiling with -fvisibility=hidden
by Jed Brown 06 Jan '14

06 Jan '14

Pavan Balaji <balaji(a)mcs.anl.gov> writes: > This attribute is tested in configure before it’s added to mpi.h.in. Yes, the configure test is incorrect and the resulting usage in MPICH is broken (does not do what you intend). > There was some breakage that Ken recently fixed. Are you using the > git mpich/master version for this? Yes, I'm using the latest 'master'. I Cc'd Ken and Antonio because they were author and reviewer on the "fixes" in late November. > Also, which version of clang is this? clang-3.3 accepts the code, but then generates calls directly to PMPI (and if you use -fvisibility=hidden, it generates R_X86_64_PC32 which fail at link time). I would have reported this as a bug to clang, but I tested clang-3.4/clang-master and it does not accept the code at all (just like gcc).

1 0

Re: [mpich-devel] Weak symbols, clang, and users compiling with -fvisibility=hidden
by Pavan Balaji 06 Jan '14

06 Jan '14

Hi Jed, This attribute is tested in configure before it’s added to mpi.h.in. There was some breakage that Ken recently fixed. Are you using the git mpich/master version for this? Also, which version of clang is this? — Pavan On Jan 6, 2014, at 1:43 PM, Jed Brown <jedbrown(a)mcs.anl.gov> wrote: > Jed Brown <jedbrown(a)mcs.anl.gov> writes: >> So in summary, we can't have __attribute__((weak,alias("PMPI_..."))) in >> the header because: >> >> 1. it does not compile with gcc > > It also does not compile with the new version of clang. > > conftest.c:37:34: error: alias must point to a defined variable or function > int foo(void) __attribute((weak, alias("__foo"))); > ^ > >> 2. it breaks PMPI interposition with clang >> 3. it breaks -fvisibility=hidden with clang > -- Pavan Balaji http://www.mcs.anl.gov/~balaji

1 0

Re: [mpich-devel] Weak symbols, clang, and users compiling with -fvisibility=hidden
by Jed Brown 06 Jan '14

06 Jan '14

Jed Brown <jedbrown(a)mcs.anl.gov> writes: > So in summary, we can't have __attribute__((weak,alias("PMPI_..."))) in > the header because: > > 1. it does not compile with gcc It also does not compile with the new version of clang. conftest.c:37:34: error: alias must point to a defined variable or function int foo(void) __attribute((weak, alias("__foo"))); ^ > 2. it breaks PMPI interposition with clang > 3. it breaks -fvisibility=hidden with clang

1 0

Re: [mpich-devel] build broken on 'master': 'mpitypedefs.h' file not found
by Rob Latham 06 Jan '14

06 Jan '14

On Sun, Jan 05, 2014 at 11:00:30PM -0600, Jed Brown wrote: > Rob Latham <robl(a)mcs.anl.gov> writes: > > Jenkins did not find a problem with this commit, so I'd like to > > understand what's different in your environment. > > I configure out of source. In this case, if jenkins does not do that i'm going to kick ken in the shins. That's right, both of them. > > Yeah, in the standalone ROMIO case I do not include that > > mpich-specific header, and MPI_AINT_CAST_TO_VOID_PTR becomes nothing > > thanks to macro magic. > > But where is MPI_AINT_CAST_TO_VOID_PTR used (on BG/Q) and why can't that > code use ADIOI_AINT_CAST_TO_VOID_PTR? I think you're looking for the opposite question. MPI_AINT_CAST_TO_VOID_PTR is used throughout the MPICH code base. ROMIO should use MPI_AINT_CAST_TO_VOID_PTR, providing it's own (stubed out?) version if that macro is not available. but whatever we do, I cannot break the build. deep shame upon me for thinking this "obviously correct" two liner did not require a bit more attention. ==rob -- Rob Latham Mathematics and Computer Science Division Argonne National Lab, IL USA

1 0

Re: [mpich-devel] [mpich-discuss] Use of Signed-off-by in MPICH
by Jed Brown 06 Jan '14

06 Jan '14

Michael Blocksome <blocksom(a)us.ibm.com> writes: > IBM uses the "signed off by" feature to enforce/track code reviews before > any code gets contributed to ANL. The intent is that the commit author != > the commit signer when the code goes out. Often a developer at ANL will > also review the code before it gets integrated into the master branch (IBM > developers don't have push authority) and sign the commit again. So the last stage (whoever makes the actual commit) is already available because Git has separate fields for Committer and Author, as shown by "git log --format=fuller". > We use the text "Signed-off-by:xxxx <yyy at zzz>" simply because this is what > is supported by the git tools. You can use commands like `git cherry-pick > -s`, `git commit --amend -s`, and `git am -s` to ease the signing effort. Yes, though this only adds your own name. Do you email patches around until then? When I started this thread, I pointed out a recent case where the author and committer were the same, but someone else was providing a Signed-off-by. Either the commit went back and forth or (more likely) the committer amended the commit to add a Signed-off-by From the reviewer. The idea with Git's -s is that you only ever sign for yourself, though if you integrate out-of-band content, you may copy the Signed-off-by tag provided by that other person. So the design of the -s option is not a perfect match, but I can see that it's often convenient. In contrast, Reviewed-by, Acked-by, and similar are almost always a result of the author or committer translating out-of-band information (e.g., an email discussion). If you want to automatically add Reviewed-by tags, you could use .git/hooks/prepare-commit-msg, for example. Of course you can keep using Signed-off-by in place of Reviewed-by, though it causes some confusion for people new to the project, attempting to track provenance when either extracting code or using submodules with other projects that use the usual conventions, or scripts providing repository analytics.

1 0

Weak symbols, clang, and users compiling with -fvisibility=hidden
by Jed Brown 06 Jan '14

06 Jan '14

MPICH's test for weak symbols is: int foo(int) __attribute__((weak,alias("__foo"))); where __foo is not defined in the same compilation unit. This fails with gcc (tested with 4.8 and 4.4): conftest.c:37:5: error: 'foo' aliased to undefined symbol '__foo' int foo(int) __attribute__((weak,alias("__foo"))); ^ This is (evidently) a change from gcc-3.x, but is intentional because knowledge of the weak alias is not desirable/meaningful in other compilation units. http://gcc.gnu.org/bugzilla/show_bug.cgi?id=20652 My understanding of gcc's intent is that the attribute only be stated in the implementation. The caller can't do anything with the information that foo is a weak alias; it still needs to generate a PLT entry and the loader will need to perform a relocation. Now my problem is that clang accepts the test code above (i.e., it does not insist that __foo be defined in the same compilation unit) which causes #define MPICH_ATTR_WEAK_ALIAS(fname) __attribute__((weak,alias(fname))) but this breaks PMPI and when -fvisibility=hidden is used to compile user code, it generates (objdump -r) RELOCATION RECORDS FOR [.text]: OFFSET TYPE VALUE 0000000000000005 R_X86_64_PC32 __foo-0x0000000000000004 instead of the linkable (but not desired because it prevents interposing "foo") RELOCATION RECORDS FOR [.text]: OFFSET TYPE VALUE 0000000000000005 R_X86_64_PLT32 __foo-0x0000000000000004 What we actually want is an honest relocation for "foo": RELOCATION RECORDS FOR [.text]: OFFSET TYPE VALUE 0000000000000005 R_X86_64_PLT32 foo-0x0000000000000004 When you try to link the first version later, you get /usr/bin/ld.gold: error: bar.o: requires dynamic R_X86_64_PC32 reloc against '__foo' which may overflow at runtime; recompile with -fPIC which is not actually an -fPIC error, but rather a result of clang using R_X86_64_PC32 for a symbol that we really want to be R_X86_64_PLT32. I think this should probably be reported to Clang/LLVM (because it delays a confusing error condition), but I also think that MPICH should fix its usage. So in summary, we can't have __attribute__((weak,alias("PMPI_..."))) in the header because: 1. it does not compile with gcc 2. it breaks PMPI interposition with clang 3. it breaks -fvisibility=hidden with clang Instead, it should go in the implementation files.

1 0

Re: [mpich-devel] [mpich-discuss] Use of Signed-off-by in MPICH
by Michael Blocksome 06 Jan '14

06 Jan '14

IBM uses the "signed off by" feature to enforce/track code reviews before any code gets contributed to ANL. The intent is that the commit author != the commit signer when the code goes out. Often a developer at ANL will also review the code before it gets integrated into the master branch (IBM developers don't have push authority) and sign the commit again. We use the text "Signed-off-by:xxxx <yyy@zzz>" simply because this is what is supported by the git tools. You can use commands like `git cherry-pick -s`, `git commit --amend -s`, and `git am -s` to ease the signing effort. Michael Blocksome Parallel Environment MPI Middleware Team Lead, TCEM POWER, x86, and Blue Gene HPC Messaging blocksom(a)us.ibm.com From: Pavan Balaji <balaji(a)mcs.anl.gov> To: discuss(a)mpich.org, Date: 01/04/2014 10:00 PM Subject: Re: [mpich-discuss] Use of Signed-off-by in MPICH Sent by: discuss-bounces(a)mpich.org Please use devel(a)mpich.org for developer oriented discussion. We use signoff as a way to get a second confirmation on the correctness of the patch. In some cases, multiple confirmations. There’s also a "silent signoff” done by the jenkins testing before things are merged into mpich/master. — Pavan On Jan 4, 2014, at 7:55 PM, Jed Brown <jedbrown(a)mcs.anl.gov> wrote: > I'm confused by the current use of Signed-off-by tags in MPICH. Is this > intended to be a developer's certificate of origin (DCO) following the > model of the Linux kernel [1]? I can't tell if IBM is using it that > way, where Michael Blocksome is the one authorized by IBM to assert on > IBM's behalf that the work has clean IP and may be distributed under the > MPICH license. But what does this mean? > > commit 87b20136ac20a0fbcffd8e29edcc500fcebbfc22 > Author: Pavan Balaji <balaji(a)mcs.anl.gov> > AuthorDate: Wed Jan 1 19:33:36 2014 -0600 > Commit: Pavan Balaji <balaji(a)mcs.anl.gov> > CommitDate: Thu Jan 2 15:39:07 2014 -0600 > > Move default eager threshold setting to ch3. > > We were setting the default eager threshold inside nemesis, instead of > in ch3. This was causing other ch3 channels, notably sock, to get a > garbage default value for the eager threshold. > > Signed-off-by: Wesley Bland <wbland(a)mcs.anl.gov> > > > If this were a DCO, we would interpret it as meaning that Wesley is the > one certifying the IP, but Pavan is the author. > > Or is Signed-off-by being used where Reviewed-by would be more > appropriate? > > > [1] https://www.kernel.org/doc/Documentation/SubmittingPatches > _______________________________________________ > discuss mailing list discuss(a)mpich.org > To manage subscription options or unsubscribe: > https://lists.mpich.org/mailman/listinfo/discuss -- Pavan Balaji http://www.mcs.anl.gov/~balaji _______________________________________________ discuss mailing list discuss(a)mpich.org To manage subscription options or unsubscribe: https://lists.mpich.org/mailman/listinfo/discuss

1 0

Re: [mpich-devel] build broken on 'master': 'mpitypedefs.h' file not found
by Jed Brown 05 Jan '14

05 Jan '14

Rob Latham <robl(a)mcs.anl.gov> writes: > On Sat, Jan 04, 2014 at 08:08:16PM -0600, Jed Brown wrote: >> Rob, this commit does not build, at least not if you configure out of >> source. > > Jenkins did not find a problem with this commit, so I'd like to > understand what's different in your environment. I configure out of source. In this case, $ mkdir build-clang $ cd build-clang $ ../configure --prefix=/home/jed/usr/mpich-clang --enable-shared --enable-error-checking=runtime --enable-error-messages=all --enable-timer-type=clock_gettime --with-python=python2 CC=clang CXX=clang++ F77=gfortran FC=gfortran [...] $ make > Yeah, in the standalone ROMIO case I do not include that > mpich-specific header, and MPI_AINT_CAST_TO_VOID_PTR becomes nothing > thanks to macro magic. But where is MPI_AINT_CAST_TO_VOID_PTR used (on BG/Q) and why can't that code use ADIOI_AINT_CAST_TO_VOID_PTR?

1 0