[PATCH] D44435: CUDA ctor/dtor Module-Unique Symbol Name

[Simeon Ehrig via Phabricator via cfe-commits]

SimeonEhrig added inline comments.


================
Comment at: lib/CodeGen/CGCUDANV.cpp:287
+    CtorSuffix.append("_");
+    CtorSuffix.append(ModuleName);
+  }
----------------
tra wrote:
> SimeonEhrig wrote:
> > tra wrote:
> > > There is a general problem with this approach. File name can contain the characters that PTX does not allow.
> > > We currently only deal with '.' and '@', but that's not enough here.
> > > You may want to either mangle the name somehow to avoid/convert illegal characters or use some other way to provide unique suffix. Hex-encoded hash of the file name would avoid this problem, for example.
> > > 
> > > 
> > > 
> > Maybe I'm wrong but I think, that should be no problem, because the generating of a cuda ctor/dtor have nothing to do with the PTX generation. 
> > 
> > The function 'makeModuleCtorFunction' should just generate llvm ir code for the host (e.g. x86_64).
> > 
> > If I'm wrong, could you tell me please, where in the source code the 'makeModuleCtorFunction' affect the PTX generation.
> You are correct that PTX is irrelevant here. I've completely missed that this will be generated for the host, which is more forgiving. 
> 
> That said, I'm still not completely sure whether we're guaranteed that using arbitrary characters in a symbol name is OK on x86 and, potentially, other host platforms. As an experiment, try using a module which has a space in its name.
At line 295 and 380 in CGCUDANV.cpp I use a sanitizer function, which replace all symbols without [a-zA-Z0-9._] with a '_'. It's the same solution like in D34059. So I think, it would works in general.

Only for information. I tested it with a module name, which includes a whitespace and without the sanitizer. It works on Linux x86 and the ELF format. There was an whitespace in the symbol of the cuda module ctor (I checked it with readelf).

In general, do you think my solution approach is technically okay? Your answer will be really helpful for internal usage in our cling project. At the moment I developed the cling-cuda-interpreter based on this patch and it would helps a lot of, if I can say, that the patch doesn't cause any problem with the CUDA-environment.  


https://reviews.llvm.org/D44435