Skip to content

llgo: fix tflag for named#86

Open
visualfc wants to merge 1 commit into
goplus:mainfrom
visualfc:llgo_flag
Open

llgo: fix tflag for named#86
visualfc wants to merge 1 commit into
goplus:mainfrom
visualfc:llgo_flag

Conversation

@visualfc

Copy link
Copy Markdown
Member

No description provided.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request modifies rtype_llgo.go by updating how t.TFlag is manipulated in setTypeName and SetUnderlying, removing GC-related functions (gcEnable, gcDisable) and their usage in setMethodSet, and adding the DirectIfaceData linkname function. The review feedback suggests removing commented-out code in SetUnderlying rather than leaving it inline.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread rtype_llgo.go
}
rt.Size_ = ort.Size_
rt.TFlag |= tflagUncommon | tflagExtraStar | tflagNamed
rt.TFlag |= tflagUncommon /*| tflagExtraStar*/ | tflagNamed

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Avoid leaving commented-out code in the source. It is cleaner to completely remove tflagExtraStar if it is no longer needed here.

Suggested change
rt.TFlag |= tflagUncommon /*| tflagExtraStar*/ | tflagNamed
rt.TFlag |= tflagUncommon | tflagNamed

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review summary

Targeted fix to tflag handling for named types in the llgo build, plus removal of the GC suppression around method-set mutation and a new DirectIfaceData linkname. The tflagExtraStar fix in setTypeName looks correct. A few points worth addressing before merge — see inline comments. Highlights:

  • SetUnderlying only stops setting tflagExtraStar, while setTypeName actively clears it. If the bit is already set on rt.TFlag (e.g. it is copied from ort.TFlag in newType at rtype_llgo.go:281), SetUnderlying will leave a stale star bit set, so the fix is incomplete there.
  • DirectIfaceData is declared via //go:linkname but never called anywhere in the repository — dead code as it stands.
  • Commented-out /*| tflagExtraStar*/ is left inline (also raised by gemini-code-assist); express the intent in code rather than a comment.

Question (not blocking)

The removal of gcDisable() / defer gcEnable() around the method-table mutation loop in setMethodSet (previously rtype_llgo.go:428-429) drops GC suppression while pointer fields of the live method table are written one at a time via unsafe pointers. Under llgo's conservative (Boehm) GC, was this verified safe — i.e. is there no longer a window where a GC cycle could observe a partially-written method entry? A short note in the commit/PR description on why this is now safe would help future readers, since there is no replacement synchronization.

Comment thread rtype_llgo.go
}
rt.Size_ = ort.Size_
rt.TFlag |= tflagUncommon | tflagExtraStar | tflagNamed
rt.TFlag |= tflagUncommon /*| tflagExtraStar*/ | tflagNamed

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two concerns on this line:

  1. Incomplete vs. setTypeName. setTypeName now actively clears the bit (t.TFlag = (t.TFlag & ^tflagExtraStar) | tflagNamed), but here you only stop setting it. If rt.TFlag already has tflagExtraStar set — e.g. it was OR-ed in from ort.TFlag in newType at rtype_llgo.go:281 — the star bit survives SetUnderlying, producing a wrong String()/Name() for the type. To mirror setTypeName and be robust, clear it explicitly:
rt.TFlag = (rt.TFlag | tflagUncommon | tflagNamed) & ^tflagExtraStar
  1. Commented-out code. /*| tflagExtraStar*/ leaves intent ambiguous (temporary? permanent?). Prefer removing it and adding a one-line comment explaining why the llgo build omits tflagExtraStar here. (Also raised by gemini-code-assist in this thread.)

Comment thread rtype_llgo.go
return
}

//go:linkname DirectIfaceData github.com/goplus/llgo/runtime/internal/runtime.DirectIfaceData

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DirectIfaceData is declared here via //go:linkname but has no caller anywhere in the repository (git grep DirectIfaceData returns only this declaration). As it stands this is dead code. If it is scaffolding for a follow-up change, consider adding it in the same commit that uses it, or add a brief comment explaining its purpose; otherwise it is liable to confuse future readers since it looks like a deliberately exposed symbol with no consumer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant