dlopen: Some cleanup and add future for line numbers#27600
dlopen: Some cleanup and add future for line numbers#27600dlongnecke-cray merged 5 commits intochapel-lang:mainfrom
Conversation
dlongnecke-cray
changed the title
| param unsupportedLlvmVersion = CHPL_LLVM_VERSION == "14" || | ||
| CHPL_LLVM_VERSION == "13" || | ||
| CHPL_LLVM_VERSION == "12" || | ||
| CHPL_LLVM_VERSION == "11"; |
There was a problem hiding this comment.
nit, drop 11, 12, and 13 here
| proc isDynamicLoadingEnabled { | ||
| return isDynamicLoadingSupported; | ||
| } |
There was a problem hiding this comment.
whats the point of this proc?
There was a problem hiding this comment.
I've kind of waffled between removing it and not, I guess I can just remove it but I can foresee cases where I want to disable dynamic loading at runtime so thought about just leaving it in.
There was a problem hiding this comment.
works for me, perhaps a comment is a good idea. Otherwise it just looks like unnecessary indirection
There was a problem hiding this comment.
I went ahead and removed it for now. I can always add it again later when I end up running into the particular roadblock I'm thinking of.
| const ret = chpl_procToTestInsertLineNumbers; | ||
| return ret; | ||
| } | ||
|
|
There was a problem hiding this comment.
I'm not sure I understand why chpl_procToTestInsertLineNumbers has to be defined in ChapelDynamicLoading, since its basically only added to support this test
There was a problem hiding this comment.
I expect this test will continue to exist in the future, so might as well have a symbol for the purpose. I can't think of a better place to put it. Maybe ChapelBase.chpl? I could co-opt an existing symbol that so happens to need line numbers, but that feels kind of strange to me.
There was a problem hiding this comment.
I'm not sure I understand why it has to be an internal symbol?
There was a problem hiding this comment.
I think it has to be a standard or internal symbol in order to activate insertLineNumbers.cpp. I guess it could also be a user-defined proc using the pragma "always propagate line file info" instead, but IDK I didn't really gravitate towards doing that.
There was a problem hiding this comment.
Ah ok. Another alternative is --prepend-internal-module-dir.
It just seems really weird to me to have a symbol in an internal module whose entire purpose is to be used to check an assertion, and assertion thar doesn't work properly yet at that.
I think there are enough alternatives here that you should use one of those to write this test, rather than hardcoding something into the module definition
There was a problem hiding this comment.
I haven't followed the issues closely enough, do line numbers work ok with the LLVM backend?
There was a problem hiding this comment.
They're backend independent, not sure why this skipif is here... 🤔
|
I think this is ready to merge, but I will hold off on that until Monday. |
Signed-off-by: David Longnecker <dlongnecke-cray@users.noreply.github.com>
Signed-off-by: David Longnecker <dlongnecke-cray@users.noreply.github.com>
Signed-off-by: David Longnecker <dlongnecke-cray@users.noreply.github.com>
Signed-off-by: David Longnecker <dlongnecke-cray@users.noreply.github.com>
dlongnecke-cray
force-pushed
the
dynamic-loading-12
branch
from
7618f49 to
13ffb8f
Compare
Signed-off-by: David Longnecker <dlongnecke-cray@users.noreply.github.com>
2 participants
This PR does a small refactor of compiler errors emitted in module code for dynamic loading and procedure pointers. It adds a self-contained future that ilustrates how procedure pointers don't work with
insertLineNumbersyet.Reviewed by @jabraham17. Thanks!