-
Notifications
You must be signed in to change notification settings - Fork 12.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ambiguity parsing local vs global symbols #1017
Comments
Here is a reduced testcase: ; RUN: llvm-as < %s | opt -strip | llvm-dis | llvm-as
|
It looks like this is going to take some significant restructuring. I'm going to defer it to 1.7. Sorry :( -Chris |
The only way to fix this reasonably is to give global symbols a different prefix (e.g. $X instead of %X). This |
Initial support for llvm-upgrade has been implemented with this patch: http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20061204/040812.html When more details are available, the upgrade will be implemented. |
Idea: Continue to use % for all global names (vars, constants, types, functions). Make |
I actually am gravitating towards the %% idea. It seems that most simple global names can use %%foo, which has the added advantage that they stand Complex global names (cases where we use "foo bar" instead of %), can use %"..". This would allow us Reasonable? -Chris |
Yup, makes sense that way too, but I'd still vote for not allowing % as a |
The problem with disallowing stuff is that it means we'd have to check it somewhere, slowing things Is there a problem with: %%G and %"G" being global and In the asmprinter, if % is used in a local symbol, you could always use "%" like we do today. Today, this is valid syntax: -Chris |
Huh? All this means is turning the Name production into a GlobalName and a You're worried about the parser doing extra checking?
No, there's nothing "wrong" with it. Parsing wise its not ambiguous, but when %G and %"G" being global and Simple rule: $ means local, % means global.
Sure.
Sure, I'm just trying to simplify things so that when someone reads the assembly But, its not a big deal. Implement what you like. |
Ok, I misunderstood what you said. I have no inherent preference for the prefix we use. The |
It doesn't have to be $. My whole point was that % and %% might confused by some Anyway, I reiterate, implement what you like. Its not that big a deal. Reid. |
What do you think of @global and %local? -Chris |
Works for me. |
Another hard case: |
The .ll file printer + asmparser bits are implemented, I'll attach a patch soon. Unfortunately I can't commit |
vmcore + asmparser patch |
Mine. |
Are module asm blocks considered global ? I'm assuming they are. |
Never mind, silly question. They don't have names :) |
This is an even harder test case (for llvm-upgrade): int %foo(int()* %P) { internal uint "XX"() { |
This is basically going to take a rewrite of llvm-upgrade. However, I'm not even void %func(int* %X) { more: next: done: %X = internal constant long 5 When we hit the more block we have two problems. First, we have no idea that These issues require llvm-upgrade to keep a model of each function until the Consequently, its going to be a while before this can get resolved. |
I assume you should use the same approach that asmparser used to. Basically, if you see a name you:
-Chris |
Nope, it doesn't. Until now it was unnecessary. I decoupled llvm-upgrade from With this change, I'm going to do exactly what you just described. Hence, the |
ok |
Note: when this lands, we should remove Function::renameLocalSymbols and all uses. This fix obsoletes -Chris |
Okay. This should land this week if I can get through the llvm-upgrade portion |
no worries, we can make the change after it lands. I just didn't want to forget about it. :) |
This has now been implemented via joint effort. The llvm-upgrade changes are in http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20070122/043254.html |
mentioned in issue llvm/llvm-bugzilla-archive#761 |
Extended Description
Please try the following with the attached unstripped.bc:
opt -strip -o stripped.bc unstripped.bc
llvm-dis stripped.bc
gccas stripped.ll
=> stripped.ll:4104: Reference to an invalid definition: #5 of type 'void () *'
The offending function now looks like this:
int %_Z20test_inline_funcdiffPFvvEPFjjE(void (), uint (uint)) {
cast void ()* %5 to int ; :0 [#uses=1]
cast uint (uint)* %2 to int ; :1 [#uses=1]
sub int %0, %1 ; :2 [#uses=1]
ret int %2
}
~Markus
The text was updated successfully, but these errors were encountered: